---
# System prepended metadata

title: Security review of EmergencyBrake.sol for Yield Dec-22
tags: [secrev, yield]

---

# 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.





