Asymmetry contest - 0xbepresent's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L228 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L116 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L73 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L92

Vulnerability details

Impact

In the Reth.sol contract the poolPrice() function helps to calculate the rETH per ETH. On the other side, in the SfrxEth.sol contract the SfrxEth.sol::ethPerDerivative() function uses the FRX/ETH curve pool to get the derivative price in terms of ETH.

Those prices extracted from the Uniswap and Curve pools are used when the users stake their ETH, so the underlying value is calculated and it is used to calculate the depositPrice and finally the depositPrice is used to calculate the mint amount of SafEth tokens.

The problem is that the Uniswap rETH/wETH pool and the Curve Frx/ETH pool can be manipulated in order to get more safETH tokens with less ETH. The attacker can manipulate the price pools with flash loans leading to mint more safETH tokens.

Proof of Concept

The Reth.sol::poolPrice() relies on Uniswap pool price.

File: Reth.sol
228:     function poolPrice() private view returns (uint256) {
229:         address rocketTokenRETHAddress = RocketStorageInterface(
230:             ROCKET_STORAGE_ADDRESS
231:         ).getAddress(
232:                 keccak256(
233:                     abi.encodePacked("contract.address", "rocketTokenRETH")
234:                 )
235:             );
236:         IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
237:         IUniswapV3Pool pool = IUniswapV3Pool(
238:             factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
239:         );
240:         (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
241:         return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
242:     }

The SfrxEth.sol::ethPerDerivative() relies on the Curve pool price.

File: SfrxEth.sol
111:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {
112:         uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
113:             10 ** 18
114:         );
115:         return ((10 ** 18 * frxAmount) /
116:             IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
117:     }

Tools used

VScode

Recommended Mitigation Steps

Secured price calculation can be performed by using time-weighted average prices (TWAPs) across longer time intervals.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L92 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L177

Vulnerability details

Impact

The Reth.sol contract is used by the protocol to manage the user's staking in that Derivative. When the Rocket pool doesn't accept deposits directly, the code uses an Uniswap swap in order to get rETH in exchange of the user's ETH. The UniswapRouter.exactInputSingle() is used to obtain the rETH tokens.

In UniSwap, there is a parameter called deadline which helps to protect against long-pending transactions and wild swings in prices.

The problem here is that the deadline parameter is not used when a swap is executing. If the deadline parameter is not present, the protocol can perform bad trades:

• The minOut can be outdated. The minOut amount can be calculated at a certain minimum acceptable amount, so the trade can be pending in the mempool for many circunstances like miners are not incentivited, and while the transaction is pending the rETH/wETH price can change and the minOut can be outdated.
• The transaction can be pending in the mempool for extended periods of time which it could be a problematic when the rETH/ETH prices changes constantly.

The deadline parameter helps to protect the protocol swap trades against fluctuation in the market prices and miners who decide how fast the transaction are executed.

Proof of Concept

The Reth.sol::swapExactInputSingleHop() function doesn't use the deadline parameter.

File: Reth.sol
083:     function swapExactInputSingleHop(
084:         address _tokenIn,
085:         address _tokenOut,
086:         uint24 _poolFee,
087:         uint256 _amountIn,
088:         uint256 _minOut
089:     ) private returns (uint256 amountOut) {
090:         IERC20(_tokenIn).approve(UNISWAP_ROUTER, _amountIn);
091:         ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
092:             .ExactInputSingleParams({
093:                 tokenIn: _tokenIn,
094:                 tokenOut: _tokenOut,
095:                 fee: _poolFee,
096:                 recipient: address(this),
097:                 amountIn: _amountIn,
098:                 amountOutMinimum: _minOut,
099:                 sqrtPriceLimitX96: 0
100:             });
101:         amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
102:     }

Tools used

VScode

Recommended Mitigation Steps

Add the deadline parameter in order to ensure that the transaction can be executed in a short period of time.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L120 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L146-L149 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L170

Vulnerability details

Impact

The Reth.sol::poolCanDeposit() function is used to determine if the users deposits are going to be via Rocket deposit pool or via swapping. The poolCanDeposit() function checks if the deposit amount is in the min/max ranges.

The problem is that rocketDAOProtocolSettingsDeposit.getDepositEnabled() is not used to determine if the deposits to the Rocket pool are enabled. The user staking will revert if the Rocket deposits are not enabled by the Rocket protocol, causing a bad experience to the stakers because the stake() function will not be available due the reverts.

Proof of Concept

The poolCanDeposit() checks in the 147-149 code lines that the deposit amount is in the min/max range but it doesn't check if the deposits are enabled via the rocketDAOProtocolSettingsDeposit.getDepositEnabled() function.

File: Reth.sol
120:     function poolCanDeposit(uint256 _amount) private view returns (bool) {
121:         address rocketDepositPoolAddress = RocketStorageInterface(
122:             ROCKET_STORAGE_ADDRESS
123:         ).getAddress(
124:                 keccak256(
125:                     abi.encodePacked("contract.address", "rocketDepositPool")
126:                 )
127:             );
128:         RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
129:                 rocketDepositPoolAddress
130:             );
131: 
132:         address rocketProtocolSettingsAddress = RocketStorageInterface(
133:             ROCKET_STORAGE_ADDRESS
134:         ).getAddress(
135:                 keccak256(
136:                     abi.encodePacked(
137:                         "contract.address",
138:                         "rocketDAOProtocolSettingsDeposit"
139:                     )
140:                 )
141:             );
142:         RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
143:                 rocketProtocolSettingsAddress
144:             );
145: 
146:         return
147:             rocketDepositPool.getBalance() + _amount <=
148:             rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
149:             _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
150:     }

Tools used

VScode

Recommended Mitigation Steps

Add a verification via rocketDAOProtocolSettingsDeposit.getDepositEnabled() if the deposits are enabled:

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

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L101 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L63

Vulnerability details

Impact

The SfrxEth.sol::deposit() is used by SafEth.sol::stake() to be able to deposit the user's ETH. The users deposits are distributed among the Derivatives based on the weight assigned to each derivative. The SfrxEth.sol::deposit() uses the frxETHMinterContract.submitAndDeposit() function to be able to mint frxETH and deposit it to receive sfrxETH in one transaction.

The problem is that the frxETHMinter._submit() function can be paused by the FRX Protocol causing a revert in all the staking process. If for some reasons the frxMinter pauses their submits all the users who wants to use SafEth.sol::stake() will be reverted causing a bad experience in the protocol because the stake() function will not be available for any other healthy Derivative.

Proof of Concept

Please see the next scenario:

1. User stakes his ETH via stake() function.
2. Some amount of ETH is going to the SfrxEth.deposit() function.
3. The frxETHMinterContract.submitAndDeposit{}()) is called.
4. Then in the frxMinterContract the _submit() function is called.
5. The _submit() function is reverted because it is paused.
6. All the transaction is reverted causing the staking to other Derivatives are not possible.

Tools used

VScode

Recommended Mitigation Steps

If the FRX Derivative has weight assigned and for some reason the FRX protocol is paused, it would be possible to maintain the ETH in the SafEth.sol contract until an admin adjust the weights and rebalance all the ETH to healthy derivatives. This prevents the stake function from not being available due to paused/unhealthy derivatives.