# Security review of EmergencyBrake.sol for Yield Dec-22 ## Review Summary From EmergencyBrake.sol natspec: > EmergencyBrake allows to plan for and execute transactions that remove access permissions for a user contract. In an permissioned environment this can be used for pausing components. The contract has a reduced attack surface as *all of the state-mutating external functions are permissioned*. Furthermore, the contract contains *no complex math* and it *does not interact with 3rd party external contracts*. This review is for a rewrite of the EmergencyBrake.sol which was [previously reviewed in October 2022](https://hackmd.io/@devtooligan/YieldEmergencyBrakeSecurityReview2022-10-11). While the logic has been changed, the general approach and many of the specific feature concepts have remained the same. The [fix/ebrake-audit-fixes](https://github.com/yieldprotocol/yield-utils-v2/pull/54) of the [yield-utils-v2](https://github.com/yieldprotocol/yield-utils-v2) repo was reviewed at commit [707d1735](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/2bade6ce3df959f952c3dfeef2ee7f2f707d1735) during the period 16-Dec-22 to 19-Dec-22 by devtooligan for a cumulative total of 8.5 hours. **No critical, high, or medium severity issues were found. Two (2) low severity findings were noted.** ## Scope The review was based on commit [6e37565](https://github.com/yieldprotocol/yield-utils-v2/pull/54/commits/6e375651bee1c08fdac74999aa99874a56d1b396) and focused exclusively on EmergencyBrake.sol. The goal of this review was to identify potential vulnerabilities in the code. Additionally, code quality improvements, readability, and best practices were considered. **Gas optimizations were **not** a significant part of this review.** ## Findings ### Low severity findings #### 1. `index()` return value collision (LOW) According to the NatSpec for [index()](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696R62-R72): ``` /// @dev Index of a permission in a plan. Returns 0 if not present. ``` However, if the index of the permission is 0 then it also returns 0. ##### Severity This makes it unclear that the permission was found or not. A caller would have to take an additional step to confirm whether or not the permission exists if 0 is returned. Because this is a view function and there is a workaround, it is a low severity. ##### Recommendation Consider reverting when a permission is not found. Alternatively, consider returning a tuple of `(bool found, uint256 index)`. ##### Developer response: [Fixed by returning type(uint).max when not found.](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/94c7137c3e1099322f303cdc3eb8fdd61220c523) ##### Reviewer response: Change reviewed. No issues noted. #### 2. `terminate()` can also erase a non-executed plan (LOW) According to the NatSpec for [terminate()](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696R161-R165): ``` /// @dev Remove the restoring option from an isolated user ``` However, there is no check that the plan is not in the `executed` state. ##### Severity This means the function will still remove the plan for the user, even if it has been executed. This makes `terminate()` overpowered and conflates the role with that of `cancel()`. Because the function does not allow for adding to a plan, the severity of this finding is low. ##### Recommendation Consider adding a check that the plan has been executed. ```solidity require(plan_.executed, "Plan not in execution"); ``` ##### Developer response: [Fixed.](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/4a8a8694de0fd214f7b39e310c9d42a155bfd361) ##### Reviewer response: Change reviewed. No issues noted. ### Informational and Gas Optimization findings #### 1. `execute()` reverts if any of the roles are already revoked When executing a plan for a user, [the logic](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696R216-R248) loops through all permissions for the user and calls `AccessControl.revokeRole()` on each. Before revoking each role, it checks if the role actually exists for the user. If any one of the roles are not found to be active, the transaction is reverted and none of the permissions in the plan are revoked. This finding does not necessarily represent a security vulnerability. The failing permission can still be removed from the plan and then executed. Therefore, this is presented as informational. ##### Developer response: If we change this so that it no longer reverts if the role doesn't exist, then we are vulnerable to an attack where undesired permissions could be enabled on a target contract. The attack is as follows: 1. Create a plan with the desired new permission. 2. Execute the plan -- it won't fail when the new permission does not exist. 3. Restore the plan. Now the new permission has been added. To address the issue raised here [we added a new function](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/48ee5c2776aa67b498e699da16043c32d9199216) called `check()` that checks if all the permissions from a plan are in place or not. ##### Reviewer response: Change reviewed. No issues noted. #### 2. Inline comment explanation for decrementing loop (INFORMATIONAL) There was a comment about the loop in `_erase` not working as expected. As of version 0.8.0, Solidity uses safe math by default and attempting to decrement an unsigned integer below zero causes a revert. In this case, at the end of the final iteration (when `i == 0`), the logic will attempt to decrement `i` once more, causing a revert. As such, the logic currently used is appropriate. The comment can be removed and no other changes are needed. ##### Developer response: [Removed comment and updated logic.](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/1d777560fbab4fc6c4364d58158bde78cf5ecb22) ##### Reviewer response: Change reviewed. No issues noted. #### 3. Use calldata instead of memory for array and struct arguments (GAS) [add()](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696R85-R115) and [remove()](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696R120-R146) both accept an array of `Permission` structs into `memory` as an argument. [permissionToId()](https://github.com/yieldprotocol/yield-utils-v2/pull/59/files#diff-f98dbb4e23f3478e2d62df0e6a0882031e65ade70765ef01602316f9204e6696L188-L192) accepts a single `Permission` struct argument, also into `memory`. Since these arguments are read only, `calldata` can be used instead of `memory` to save runtime gas. ##### Developer response: [Fixed.](https://github.com/yieldprotocol/yield-utils-v2/pull/59/commits/b162fbea1ac74bdd52ed756cd4e141b5d4c2df5b) ##### Reviewer response: Change reviewed. No issues noted.
Last changed by  
 
devtooligan
172

Read more

Guide to Proxies

This research was contributed to the yAcademy Proxies Research page.

2/5/2024
Security review of Strategy.sol V2 for Yield

Review Summary From Strategy.sol natspec: The Strategy contract allows liquidity providers to provide liquidity in yieldspace pool tokens and receive strategy tokens that represent a stake in a YieldSpace pool contract. Upon maturity, the strategy can divest from the mature pool, becoming a proportional ownership underlying vault. When not invested, the strategy can invest into a Pool using all its underlying. The Strategy.sol contract has a reduced attack surface as most of the external functions are permissiond and/or restricted by current state through the use of a state machine. The feat/strategy-pool-only branch of the strategy-v2 repo was reviewed during the period 28-Nov-22 to 07-Dec-22 by devtooligan for a cumulative total of 35 hours.

12/19/2022
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 of the 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.

12/19/2022
Spearbit writing exercise

devtooligan Introduction Spearbit Writing Exercise Wallet Protocol You are given an implementation for a smart contract wallet. There are two contracts Implementation.sol: Deployed once and used as implementation contract in Proxy.sol. Proxy.sol: Each user has a unique Proxy deployment with the above implementation. This is a simply proxy contract which delegatecalls the implementation contract. It has an access control check that allows only the owner to use the fallback function. The idea is that users can keep their funds, for example, ETH or ERC20 tokens in the Proxy. To use these funds, users can execute arbitrary calls and arbitrary delegatecalls by using the implementation contract (it has callContract and delegatecallContract). The implementation contract is deployed only once and reused to save gas.

11/11/2022
Read more from devtooligan
Published on HackMD