`SfrxEth.ethPerDerivative` function returns how much frxETH that 10 ** 18 sfrxETH are worth, which can cause `minOut` in `SfrxEth.withdraw` function to be denominated in frxETH instead of ETH and slippage control to be incorrect

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L60-L88 https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577#code#L516

Vulnerability details

Impact

The following SfrxEth.ethPerDerivative function returns how much frxETH that 10 ** 18 sfrxETH are worth.

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

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

In the following SfrxEth.withdraw function, since minOut is set to (((ethPerDerivative(_amount) * _amount) / 10 ** 18) * (10 ** 18 - maxSlippage)) / 10 ** 18, minOut is denominated in frxETH instead of ETH. Such minOut is used to call IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) for exchanging frxETH for ETH.

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

    function withdraw(uint256 _amount) external onlyOwner {
        IsFrxEth(SFRX_ETH_ADDRESS).redeem(
            _amount,
            address(this),
            address(this)
        );
        uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        IsFrxEth(FRX_ETH_ADDRESS).approve(
            FRX_ETH_CRV_POOL_ADDRESS,
            frxEthBalance
        );

        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

For the corresponding frxETH-ETH pool contract's exchange function below, the _min_dy input is the minimum amount of the j input's asset to receive. When calling IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut), the asset corresponding to the j input should be ETH and the _min_dy input needs to be the minimum amount of ETH to receive. However, minOut is denominated in frxETH instead of ETH. If 1 frxETH is worth more than 1 ETH at this moment, then _min_dy based on minOut denominated in frxETH is less than _min_dy based on minOut denominated in ETH. Yet, because minOut, which is denominated in frxETH instead of ETH, is used, frxETH can be exchanged to ETH at a very suboptimal exchange rate when IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) is called. As a result, the slippage control becomes incorrect, and users lose ETH when they should not.

https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577#code#L516

@payable
@external
@nonreentrant('lock')
def exchange(i: int128, j: int128, _dx: uint256, _min_dy: uint256) -> uint256:
    ...
    @param j Index valie of the coin to recieve
    ...
    @param _min_dy Minimum amount of `j` to receive
    ...
    assert dy >= _min_dy, "Exchange resulted in fewer coins than expected"

    ...

    coin = self.coins[j]
    if coin == 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE:
        raw_call(msg.sender, b"", value=dy)
    else:
        ...
    ...

Proof of Concept

The following steps can occur for the described scenario. 1.The SfrxEth contract is the only derivative so far.

2. Alice calls the SafEth.unstake function, which further calls the SfrxEth.withdraw function.
3. When calling the SfrxEth.withdraw function, minOut is calculated to be 10e18 frxETH. When minOut is 10e18, calling IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) allows 10e18 ETH to be exchanged and eventually sent to Alice.
4. Yet, at this moment, 1 frxETH is worth 1.1 ETH so 10e18 frxETH can be converted to 11e18 ETH. If IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) can be called with minOut being 11e18, such function call should revert if only 10e18 ETH can be exchanged.
5. Because receiving 10e18 ETH is very suboptimal and is a result of the incorrect slippage control, Alice loses ETH when she should not.

Tools Used

VSCode

Recommended Mitigation Steps

The SfrxEth.ethPerDerivative function can be updated to return how much ETH instead of frxETH that 10 ** 18 sfrxETH are worth.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

toshiSat marked the issue as sponsor disputed

[toshiSat]

We are returning value in ETH due to price oracle

[c4-judge]

Picodes marked issue #1108 as primary and marked this issue as a duplicate of 1108

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #641

[c4-judge]

Picodes marked the issue as satisfactory

[toshiSat]

@Picodes the warden incorrectly assumes we are returning in FRX domination, but we aren't, it's in ETH

[rbserver]

Hi @toshiSat, sorry for any confusion. Perhaps, taking a look at the duplicates of this issue's primary issue like #141 can help. Basically, in the SfrxEth.ethPerDerivative function, dividing (10 ** 18 * frxAmount) by IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle() does not return a value that is denominated in ETH. Because (10 ** 18 * frxAmount) / IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle() would not be denominated in ETH, it can be seen as (scalar_1 * frxAmount) / scalar_2 so I mentioned that a value that is denominated in frxETH instead of ETH is used. Thanks!