Asymmetry contest - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Inflates or lowers the price of SfrxEth / ETH depending on the oracle price

Proof of Concept

    /**
        @notice - Get price of derivative in terms of ETH
     */
    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());
    }

To get price SfrxEth / ETH we should calculate (SfrxEth / FrxEth) * (FrxEth / ETH). But in return formula we divide instead of multiply. For easy understanding suppose 1 SfrxEth costs 5 FrxEth and 1 FrxEth costs 0.2 ETH. It results in price SfrxEth / ETH = 5 / 0.2. Which means 25 instead of 1

Tools Used

Manual review

Recommended Mitigation Steps

Refactor the formula

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

Lines of code

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

Vulnerability details

Impact

Overflow in poolPrice() leads to revert and blocking of stake and unstake

Proof of Concept

        IUniswapV3Pool pool = IUniswapV3Pool(
            factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
        );
        (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
        return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

_sqrtPriceX96ToUint will only work when the non-fractional component of sqrtPriceX96 takes up to 2 bits. This represents a price ratio of 16. Market conditions are different, and can happen

Tools Used

Manual Review

Recommended Mitigation Steps

Perform multiplication after converting numbers to 60x18

Lines of code

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

Vulnerability details

Impact

Function returns price of WstEth in terms of stEth. But must be in terms of ETH.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Convert amount of stEth into ETH via price_oracle in LIDO_CRV_POOL

Mark SafEthStorage as abstract

This contract is dependent on SafEth.sol and provides read-only functionality, there is know sense in deploying it

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future. 1 instance: SafEthStorage

Use Ownable2StepUpgradeable instead of OwnableUpgradeable contract for SafEth

There is another Openzeppelin Ownable contract (Ownable2StepUpgradeable.sol) has transferOwnership function, use is more secure due to 2-stage ownership transfer.

Lack of functions for sending randomly received ETH and ERC-20 (apart from the derivatives themselves)

In WstEth.sol send balanceAfter - balanceBefore, but not all balance

        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
        uint256 balanceBefore = address(this).balance;
        IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
        uint256 balanceAfter = address(this).balance;
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: balanceAfter - balanceBefore}(
            ""
        );

For resque ERC-20 add

        uint256 balancePre = IERC20(STETH_TOKEN).balanceOf(address(this));
        IWStETH(WST_ETH).unwrap(_amount);
        uint256 balancePost = IERC20(STETH_TOKEN).balanceOf(address(this));
        uint256 stEthBal = balancePost - balancePre
        IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);

And by analogy provide resque for all derivatives

Unnecessary operation

Reth

        else return (poolPrice() * 10 ** 18) / (10 ** 18);

Return poolPrice() without unnecessary mul and div