Asymmetry contest - aviggiano's results

Lines of code

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

Vulnerability details

Impact

The function Reth.ethPerDerivative function is used by Steth.stake to attribute shares to a user after they have deposited ETH to the Reth contract. The problem is that this function may yield different results depending on the result of Reth.poolCanDeposit function, which checks whether or not rETH deposit pool has room users amount, and uses either Rocket Pool or Uniswap. It is possible that, during derivative.deposit{value: ethAmount}();, Reth.poolCanDeposit returns true, while during derivative.ethPerDerivative, Reth.poolCanDeposit returns false. This will lead to a higher or lower share amount than what the user is owed.

Proof of Concept

1. User stakes ETH into SafEth
2. SafEth uses Reth as one of the derivatives. When depositing, poolCanDeposit returns true, as the Rocket Pool can receive the ether amount. The user's ether is deposited using Rocket pool
3. After the deposit happens, the SafEth calls ethPerDerivative to calculate how much derivative was received per user ether amount. This time, poolCanDeposit returns false, and the returned value of ethPerDerivative is Uniswap's poolPrice.
4. As a result, the user may receive a higher or lower value of shares than the correct amount of ether actually deposited.

Tools Used

Manual review

Recommended Mitigation Steps

Change the derivative's deposit function to return a tuple with amount of ether deposited and amount of derivative received (for example, function deposit() external payable onlyOwner returns (uint256 derivativeAmount, uint256 receivedEthValue). This will abstract away from the SafEth contract the internal calculations of each derivative contract.

Lines of code

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

Vulnerability details

Impact

Some derivative contracts use price pool as price oracles, which is vulnerable price manipulation attacks, such as flash loan attacks and read-only reentrancy attacks.

For example, the function SfrxEth.ethPerDerivative (used by both SafEth.stake and SafEth.unstake) uses curve's FRX/ETH pool to calculate the conversion between ETH and frxETH. As Curve has recently published, together with ChainSecurity's finding, there is a security concern where a well capitalized actor or a flash loan enabled adversary can execute a large swap on Curve to temporarily distort the prices of the two coins involved as compared to the wider market prices for those assets.

This means that an attacker can manipulate the price of the derivative in relationship to ETH to their advantage, either in stake or unstake.

Proof of Concept

1. Attacker calls SafEth.stake and deposits some ETH, getting some SafEth tokens in return
2. Attacker executes a flash loan and manipulates the price of the curve pool
3. Attacker calls SafEth.unstake and drains the SfrxEth vault, as the ethPerDerivative value will be incorrect, withdrawing more ETH than what they should.
4. Attacker repays the flash loan and profits ETH on the expense of other users.

Tools Used

Manual review

Recommended Mitigation Steps

• Curve recommends that DeFi Protocols use Chainlink Price Feeds as a way to eliminate their exposure to flash loan attacks. The security concerns around using single price sources in DeFi is becoming increasingly clear and they strongly recommend that DeFI protocol relying on Curve LPs take their correct use of oracles and exposure to flash loan attacks seriously.
• Include reentrancy guards in stake/unstake functions.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L95

Vulnerability details

Impact

Because of rounding errors on SafEth.stake, the total ether amount passed to derivative contracts will not always be equal to msg.value. As a result, some ether will be stuck on the SafEth contract.

Proof of Concept

1. User deposits 1 ether
2. There are three derivatives, each one with equal weight
3. Each derivative contract will receive msg.value/3 ether
4. The SafEth ether will be stuck with 1 wei

Tools Used

Manual review

Recommended Mitigation Steps

Refund the user back with the amount of ether that was not used to fund the derivative contracts.