# Security review of EmergencyBrake.sol for Yield Oct-22 ## Review Summary From EmergencyBrake.sol natspec: > EmergencyBrake allows to plan for and execute transactions that remove access permissions for a target contract. In a permissioned environment this can be used for pausing components. The contract has a reduced attack surface as *all of the external functions are permissioned*. Furthermore, the contract contains *no complex math* and it *does not interact with 3rd party external contracts*. The [472-wrap-or-refactor-ebrake branch](https://github.com/yieldprotocol/yield-utils-v2/pull/54) of the [yield-utils-v2](https://github.com/yieldprotocol/yield-utils-v2) repo was reviewed during the period 20-Sep-22 to 22-Sep-22, and 10-Oct-22 by devtooligan for a cumulative total of 16 hours. **No critical or high impact issues were found and 2 medium findings were reported.** ## Scope The review was based on commit [6e37565](https://github.com/yieldprotocol/yield-utils-v2/pull/54/commits/6e375651bee1c08fdac74999aa99874a56d1b396) and focused primarily on EmergencyBrake.sol but relevant parts of AccessControl.sol were also reviewed. The goal of the review was to identify potential vulnerabilities in the code. As part of this review, orchestration of deployed EmergencyBrake (aka "Cloak") contracts on Mainnet and Arbitrum were also reviewed. Additionally, code quality improvements, readability, and best practices were considered. Gas optimizations were **not** a significant part of this review although a couple of basic optimizations are referenced. ## Note on Developer Response The developer generally agreed with the findings presented. Based on some of the findings here and also some new ideas, the developer decided to rewrite the `EmergencyBrake.sol` contract. As such, specific responses to individual findings are not recorded on this report. A [separate security review](https://hackmd.io/@devtooligan/YieldEmergencyBrakeSecurityReview2022-12-201) was performed on the new work. ## Findings ### Medium impact findings #### 1. Permissions on currently deployed contracts don't follow security procedures (MEDIUM) According to the top [level comment on EmergencyBrake.sol](https://github.com/yieldprotocol/yield-utils-v2/blob/472-wrap-or-refactor-ebrake/contracts/utils/EmergencyBrake.sol#L28&L29): > In addition, there is a separation of concerns between the planner and the executor accounts, so that both of them must be compromised simultaneously to execute non-approved emergency plans, and then only creating a denial of service. This is an appropriate policy with proper segregation of concern. However, looking at the contracts currently deployed, this is not the case. The [Yield Info site](https://info.yieldprotocol.com/contracts/roles/Cloak) lists the current permissions on Mainnet as follows: | ROLE | USERS | | -------- | -------- | |admin| <timelock address>| |plan|<timelock address>| |cancel|<timelock address>| |execute|<timelock address>| ||0xC7aE076086623ecEA2450e364C838916a043F9a8| ||0x2bBA525c4891603C4c37F635d899918B6a23aCFf| ||0x06FB6f89eAA936d4Cfe58FfA071cf8EAe17ac9AB| ||0xfe90d993367bc93D171A5ED88ab460759DE2bED6| ||0x02f73B54ccfBA5c91bf432087D60e4b3a781E497| ||0xa1a58E83AD7367f04501d454c7d010AB01C29d04| ||0x9152F1f95b0819DA526BF6e0cB800559542b5b25| ||0x08462a2a815FE5B65D67AaC8d928C21Be2f9dDAE| ||**0xd659565b84BcfcB23B02ee13E46CB51429F4558A**| |restore|<timelock address>| ||**0xd659565b84BcfcB23B02ee13E46CB51429F4558A**| |terminate| <timelock address>| The timelock has been granted all of the roles, and the address starting with **0xd659** has been granted both the `execute` and `restore` role. ##### Severity The separation of "planner" and "executor" has not been respected for **0xd659** as well as the Timelock. As such, this is considered a medium severity finding. ##### Recommendation When the new `EmergencyBrake.sol` is deployed, ensure it is properly orchestrated in accordance with the afforementioned plan. 1. Ensure timelock is not granted `execute` role. This is also unecessary because the timelock could not execute a plan in a timely fashion anyway. 2. It is a great idea to have multiple executors, but make sure that there is no overlap with any planning roles. 3. It is smart to have an account beside the timelock granted the `restore` role, but ensure that account is not also granted the `execute` role. 4. Consider replacing or adding an additonal admin role that is a multi-sig. This would allow for quickly revoking roles if an account were compromised as well as granting some of the other roles to alternate accounts if necessary. ----------------------------- #### 2. `cancel()` and `terminate()` do not actually delete the entire plan (MEDIUM) Both `cancel()` and `terminate()` use Solidity's `delete` to erase the plan from storage: ```solidity /// @dev Erase a planned access removal transaction function cancel(address target) external override auth { require(plans[target].state == State.PLANNED, "Emergency not planned for."); delete plans[target]; emit Cancelled(target); } ``` But `delete` does not actually reset mappings as noted in the [Solidity docs](https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=delete#delete): > delete has no effect on mappings (as the keys of mappings may be arbitrary and are generally unknown). So if you delete a struct, it will reset all members that are not mappings and also recurse into the members unless they are mappings. #### Severity With the current implementation this could brick both the `addToPlan()` and `removeFromPlan()` fns if the index stored was greater than the length of permissions. It could also permanently prevent adding a `Permission` that contained a user that was still in the `permissions` mapping. As such, this finding is a medium severity. #### Recommendation Before invoking the `delete` command, iterate through and explicitly remove each mapping first. Then safely use `delete`. ------- ### Low impact findings #### 1. Add check for zero permissions length in plan. (LOW) Currently, a plan with no permissions passed in can be planned (or executed). Severity on current implementation would be waste of gas with unknown effects on future implementations. #### Recommendation Add a `require(_permissions.length > 0)` type check at the beginning of `plan()` and `require(_permissions.length > 1)` in `removeFromPlan()`. ### Informational findings #### 1. `Plan` struct suboptimal (INFORMATIONAL) [Current implementation](https://github.com/yieldprotocol/yield-utils-v2/pull/54/files#diff-1dd6039a65db9d81618e99ded9385a8afc0c6786f6dff0bdd7dc8eb1470f4ea5R33-R37): ```solidity struct Plan { State state; Permission[] permissions; mapping(bytes32 => uint256) index; } ``` This pattern is unconventional. It requires 2 SLOADs to get to a permission. Furthermore it requires an extra step to differentiate between a 0 index and an unused id in the `index` mapping. Consider using a pattern like this instead: ```solidity struct NewPlan { State state; bytes32[] ids; mapping(bytes32 => Permission) permissions; } ``` If the suggested pattern is adopted, it will have to be implemented throughout the contract. #### 2. `_permissionToId()` logic can be simplified (INFORMATIONAL) Currently the [logic to create a unique key](https://github.com/yieldprotocol/yield-utils-v2/pull/54/files#diff-1dd6039a65db9d81618e99ded9385a8afc0c6786f6dff0bdd7dc8eb1470f4ea5R204) is: ```solidity id = (bytes32(permission.signature) >> 160 | bytes32(bytes20(permission.contact))); ``` This can be simplified to: ```solidity id = bytes32(abi.encodePacked(permission.signature, permission.contact)); ``` #### 3. Use of "signature" in comment unclear and inconsistent (INFORMATIONAL) As [signature can be confused with function signature](https://github.com/yieldprotocol/yield-utils-v2/pull/54/files#diff-1dd6039a65db9d81618e99ded9385a8afc0c6786f6dff0bdd7dc8eb1470f4ea5R36 ), prefer `id` as is used elsewhere in the contract. #### 4. Remove natspec comment "retains order" (INFORMATIONAL) [This comment](https://github.com/yieldprotocol/yield-utils-v2/pull/54/files#diff-1dd6039a65db9d81618e99ded9385a8afc0c6786f6dff0bdd7dc8eb1470f4ea5R102) appears to be erroneous as the order is not retained: ```solidity /// @dev retains the order of permissions and updates their index ``` #### 5. Permissions struct item "contact" is potentially confusing (INFORMATIONAL) The use of the term `contact` for the Permission item is unconventional and could be confused with mispelled contract. Consider using `account`, `user` or some other term instead. #### 6. Enhance natspec (INFORMATIONAL) Consider adding natspec for structs and events, in addition to all external functions. ### Gas optimizations #### 1. Use `calldata` (GAS) To save gas, use calldata instead of memory for `plan()` [permissions arg](https://) and `addToPlan()` newPermission arg. #### 2. Cache array length to save gas (GAS) From [plan()](https://github.com/yieldprotocol/yield-utils-v2/pull/54/files#diff-1dd6039a65db9d81618e99ded9385a8afc0c6786f6dff0bdd7dc8eb1470f4ea5R62): ```solidity for (uint256 i = 0; i < permissions.length; ++i){ ``` can be changed to: ```solidity uint256 permissionsLength = permissions.length; for (uint256 i = 0; i < permissionsLength; ++i){ ```