Asymmetry contest - Franfran's results

Lines of code

https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L78 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L80 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/token/RocketTokenRETH.sol#L39 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L186-L203 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92-L94

Vulnerability details

Impact

When users are staking funds using the stake() function, the value of their newly minted derivative tokens is calculated according to their underlying values in ETH by calling the ethPerDerivative() view function, which is common to all derivatives. This one takes into account the amount of derivates that have been deposited just previously. The issue is that this value calculation is done too late. It is done for the "next staker", leading to wrong values calculations.

Proof of Concept

Currently, assymetry finance has added 3 derivatives:

• Reth (liquid staking from Rocket pool)
• SfrxEth (ETH from FRAX finance)
• WstEth (wrapped ETH of Lido liquid staking)

In order to calculate their value in ETH, it uses the ethPerDerivative() function.

For SfrxEth and WstEth, it's more straightforward. SfrxEth price is calculated using the curve price TWAP, whereas WstEth is a ratio between 1 ETH and 1 WstEth. Reth, though, is different. We first need to check if the asset can be deposited according to two rules from RocketPool:

1. should deposit more than minimum
2. should not exceed pool capacity

If those two rules pass, then we ask the rETH contract for its exchange rate. Else, we get the spot price from Uniswap. The issue is that those two price sources will never be 100% the same, altough they will tend to with arbitrage opportunities which won't stay for long.

Let's consider this scenario (all the values have been chosen arbitrarly and for the sake of this POC).

1. Bob deposits 100 ETH into the Assymetry Finance protocol
2. At the time of the deposit, the pool capacity in rETH is 1000 ETH, with currently 850 ETH into the pool
3. When the rETH price calculation is done in the stake(), it's taking the "poolCanDeposit()" path, worthing a price X (the rocket deposit pool current exchange rate)
4. Once the ethPerDerivative() is then called, the pool cannot contain so much ETH, or would be exceeded by 50 ETH. So it takes the "Uniswap spot price" path and return a price Y
5. As it is likely that X != Y because of arbitrage and rocket pool incentives, Bob won't receive the amount of rETH that was awaited

If Bob receives too much tokens, then there is a leak of value for the protocol. If Bob receives too few tokens, Bob won't be happy.

Tools Used

Manual inspection

Recommended Mitigation Steps

Make sure to get the value in ETH from ethPerDerivative() before depositing

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L57 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L60

Vulnerability details

Impact

When a user wants to withdraw some derivative assets for their ETH back from the wstETH derivative (The wrapped token of the stETH from Lido), the derivative contract is unwrapping the assets (convert from wstETH => stETH), and then using the ETH <> stETH curve pool in order to swap them back to ETH. As a slippage protection, the minOut variable is calculated with a 1% slippage to avoid any malicious attacker to frontrun the pool swap, or to unadvertly buy the token after that a whale would have dumped it.

The issue lies in the fact that this variable is calculated from the stETH balance, while it should be calculated from an amount in ETH. The assumption that 1 stETH == 1 ETH is wrong because the peg cannot be perfect, and in some stormy market conditions it can get worse. For reference, the 24h price range of the stETH went as low as 0.9865 ETH and as high as 1.0431 ETH from the coinmarketcap website on 03/28/23. This is a 1.3% undershoot compared to a perfect peg of 1:1 while in some fairly stable market conditions, and is over the 1% slippage check by 30%. This means that once the stETH price is not in peg, any curve swap will fail and that will DOS any withdrawals. We can thus assert about the likelyhood of this issue.

Tools Used

Manual inspection

Recommended Mitigation Steps

Get the undelying value in ETH of the stETH, and apply the slippage calculation on that.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol

function withdraw(uint256 _amount) external onlyOwner {
    IWStETH(WST_ETH).unwrap(_amount);
    uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this));
    IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);
	uint256 stToEthBal = ethPerDerivative(stEthBal);
    uint256 minOut = (stToEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
    IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L198 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L77 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L156 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol#L54-L57 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L120-L150

Vulnerability details

Impact

When depositing rETH assets, the Rocket deposit pool is called. From the source code, we can see that there is a check for the current state of deposits. This feature from RocketPool is to make sure that no-one will ever deposit into the deposit pool if deposits are disabled.

As we can see from the deposit() function of the rETH derivative, it first check if the pool cannot deposit, i.e if the amount sent to deposit is correct (not too high, not too small), and that the pool capacity hasn't been reached yet. If it cannot deposit, then it proceeds to use the rETH/ETH liquidity pool not to break the contract if the pool is full, but it's missing this disabled pool check.

As this is not checked, then in the case if the pool isn't full and that it's still disabled (which is a value that can be arbitrarly set by the rocket dao and retrieved here), the check will pass and will try to deposit on the rocket pool. This will fail and will completely freeze any deposit from the SafEth contract.

Tools Used

Manual inspection

Recommended Mitigation Steps

Make sure to check if the rocket pool is still enabled in order to proceed to the deposit. Else, swap as the other path.

Instance: https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L120-L150

function poolCanDeposit(uint256 _amount) private view returns (bool) {
    address rocketDepositPoolAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked("contract.address", "rocketDepositPool")
            )
        );
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
            rocketDepositPoolAddress
        );

    address rocketProtocolSettingsAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked(
                    "contract.address",
                    "rocketDAOProtocolSettingsDeposit"
                )
            )
        );
    RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
            rocketProtocolSettingsAddress
        );

    return
        rocketDepositPool.getBalance() + _amount <=
        rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
        _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit() &&
		rocketDAOProtocolSettingsDeposit.getDepositEnabled();
}

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L156 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L82 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol#L39-L43

Vulnerability details

Impact

The deposit() function in the rETH derivative contract calls the RocketPool pool deposit() function in order to deposit the ETH tokens to the pool if the latter still have some space in it.

This deposit function charges a fee which is currently at 0.0005 ETH as can be read from the RocketDAOProtocolSettings's getDepositFee() that is deducted from the staked amount.

This may seem to be a negligible amount, but keep in mind that this fee can be arbitrarly set by the Rocket DAO to any value. That means that if the fee gets too high, then depositors may receive much less rETH than expected because those would be deducted as fees from the pool.

Tools Used

Manual inspection

Recommended Mitigation Steps

There could be a slippage check provided by the user in each deposit of derivative in the stake() function.

Findings summary

Findings details

[G-1] Duplicated require()/revert()/assert() checks should be refactored to a modifier or an internal function

Duplicated require, revert, or assert messages should be refactored to an internal or private function in order to save some gas on deployment. The more duplicated it is, the greater the savings, but this always saves gas starting from two duplication.

contracts/SafEth/derivatives/Reth.sol 200:12

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L200

              uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));
>             require(rethBalance2 > rethBalance1, "No rETH was minted");
              uint256 rethMinted = rethBalance2 - rethBalance1;

Gas saved (per instance on deployment)

20094

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L66

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L77

</details>

[G-2] Use custom errors instead of revert strings

Solidity 0.8.4 added the custom errors functionality, which can be used instead of revert strings, resulting in big gas savings on errors since they all just consist of the 4 bytes signature of the error, similar to functions. They can then be decoded thanks to the ABI.

contracts/SafEth/SafEth.sol 64:8

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L64

      function stake() external payable {
>         require(pauseStaking == false, "staking is paused");
          require(msg.value >= minAmount, "amount too low");

Gas saved (per instance)

254

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L200

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L87

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L113

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L77

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L66

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L66

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L109

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L65

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L127

</details>

[G-3] Setting the constructor to payable

Marking the constructor as payable removes any value check from the compiler and saves some gas on deployment.

contracts/SafEth/derivatives/WstEth.sol 24:4

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L24

      /// @custom:oz-upgrades-unsafe-allow constructor
>     constructor() {
>         _disableInitializers();
>     }
  

Gas saved (per instance)

216

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L38

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L27

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L33

</details>

[G-4] Avoid using public for immutable/constant state variables

Public state variable are generating a getter function which costs more gas on deployment. Some variables may not need to require a getter function.

contracts/SafEth/derivatives/SfrxEth.sol 18:4

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L18

          0x5E8422345238F34275888049021821E8E08CAa1f;
>     address public constant FRX_ETH_CRV_POOL_ADDRESS =
>         0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577;
      address public constant FRX_ETH_MINTER_ADDRESS =

Gas saved (per instance on deployment)

24809

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L26

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L16

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L15

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L20

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L20

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L24

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L17

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L22

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L13

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L14

</details>

[G-5] Use calldata instead of memory for external function parameters

By the use of the memory keyword, all of the variables from the function parameter are copied to the memory by using the opcode CALLDATACOPY. This opcode gas cost grows linearly as a function of the number of slots to copy plus the memory expansion cost which can grow quadratically if there are a lot of slots to copy. If there is no need to alter the variables and store them somewhere, then we can safely load them from the calldata. See on evm.codes for more informations: https://www.evm.codes/#37

contracts/SafEth/SafEth.sol 50:8

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L50

          string memory _tokenName,
>         string memory _tokenSymbol
      ) external initializer {

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L49

</details>

[G-6] Functions guaranteed to revert for normal users should be marked as payable

When a function is restricted to only one account, it's less expensive to mark it as payable since it's going to remove any opcode relative to the msg.value check

contracts/SafEth/derivatives/SfrxEth.sol 60:48

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L60

       */
>     function withdraw(uint256 _amount) external onlyOwner {
          IsFrxEth(SFRX_ETH_ADDRESS).redeem(

Gas saved (per instance)

24

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L214

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L232

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L185

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L138

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L48

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L107

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L205

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L58

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L56

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/WstEth.sol#L73

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L223

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L51

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L241

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L168

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/SfrxEth.sol#L94

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L156

</details>

[G-7] use ternary operators rather than if/else

The if/else statement runtime gas cost is higher than a ternary operator in some cases.

contracts/SafEth/derivatives/Reth.sol 212:8

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L212

      function ethPerDerivative(uint256 _amount) public view returns (uint256) {
>         if (poolCanDeposit(_amount))
>             return
>                 RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
>         else return (poolPrice() * 10 ** 18) / (10 ** 18);
      }

Gas saved (per instance)

13

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L79

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/derivatives/Reth.sol#L170

</details>

[G-8] No need to initialize variables to their default values

In the EVM, everything is an empty word (0) of 32 bytes. Variables in Solidity does not need to be assigned explicitely to their default value as they already have these values and incurs a higher gas overhead without the optimizer turned on.

contracts/SafEth/SafEth.sol 71:13

https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L71

          // Getting underlying value in terms of ETH for each derivative
>         for (uint i = 0; i < derivativeCount; i++)
              underlyingValue +=

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L113

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L84

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L140

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L191

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L147

• https://github.com/code-423n4/2023-03-asymmetry/tree/main/contracts/SafEth/SafEth.sol#L171

</details>

[G-9] Cache rocket-style string encoded into immutables

RocketPool introduced the use of keccak-ed concatenated strings for their eternal storage. This ensure not to have any conflict with any other contract because those are unique. They also also allow to set any key to any value and their library provide helpers to cast the variables to the correct types. But this can be optimized by writing it as an immutable, since Solidity supports it and will be evaluated at compile-time.

contracts/SafEth/derivatives/Reth.sol 70:25

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L70

            RocketStorageInterface(ROCKET_STORAGE_ADDRESS).getAddress(
>               keccak256(
>                   abi.encodePacked("contract.address", "rocketTokenRETH")
>               )
            );

Gas saved (per instance)

144

<details> <summary>Locations</summary> <br>

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L124-L126
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L135-L140
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L161-L163
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L190-L192
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L232-L234

</details>