Derivative Pool Issue can Lead to Loss User Funds when Unstaking

[code423n4]

Lines of code

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

Vulnerability details

Impact

In all withdraw() functions of derivatives, there is no check for sending zero Ether back to the safEth contract. It is important to note that the address(msg.sender).call{value: 0}("") function returns true even when transferring a zero value.

On the other hand, the safEth contract does not perform a zero check when receiving Ether from the derivatives. As a result, when a user calls the unstake() function, if there is any issue while withdrawing from an external liquidity staking derivative contract, the user's safEth tokens will be burnt without receiving any Ether compensation in return. This situation may lead to a partial or full loss of users' funds.

Proof of Concept

The test attempts to make a regular call to the stake() function and unstake() function. Afterward, it compares the Ether balance difference, which should be within 1% of the slippage loss.

To emulate potential issues with one of the external liquidity staking derivatives, comment out the line https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L108 in the withdraw() function of Reth.sol like this:

/**
    @notice - Convert derivative into ETH
 */
function withdraw(uint256 amount) external onlyOwner {
    // RocketTokenRETHInterface(rethAddress()).burn(amount);
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}

By commenting out the line responsible for sending Ether back, you can simulate a situation where the derivative fails to return the expected Ether amount. This will help you observe the impact on the unstake() function and ensure that the implemented mitigation steps are effective in preventing the burning of safEth tokens.

To test this scenario in Foundry, run the command forge test -vv --match-test testFailUnstake.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.13;

import "ds-test/test.sol";
import "./cheatcodes.sol"; // CheatCodes interface from https://book.getfoundry.sh/cheatcodes/
import "../../contracts/SafEth/SafEth.sol";
import "../../contracts/SafEth/derivatives/Reth.sol";
import "../../contracts/SafEth/derivatives/SfrxEth.sol";
import "../../contracts/SafEth/derivatives/WstEth.sol";
import "../../contracts/interfaces/IDerivative.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

contract SafEthTest is Test {
    uint256 private constant DEFAULT_MAX_SLIPPAGE = 1e16; // 1%
    uint256 private constant DEFAULT_DERIVATIVE_WEIGHT = 1e18;
    uint256 private TEST_DEPOSIT_WEI = 1 ether;
    CheatCodes private cheats;
    address private admin;
    SafEth private safEth;
    Reth private reth;
    SfrxEth private sfrxEth;
    WstEth private wstEth;
    mapping(uint256 => IDerivative) private derivatives;

    // Can receive unstaked Ether
    receive() external payable {}

    function setUp() public {
        // Test on mainnet fork
        cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
        cheats.createSelectFork("mainnet");
        // Set proxy owner address
        admin = vm.addr(1337);

        // safEth token deploy
        safEth = SafEth(
            payable(
                new TransparentUpgradeableProxy(
                    address(new SafEth()),
                    admin,
                    abi.encodeWithSelector(
                        SafEth.initialize.selector,
                        "Asymmetry Finance ETH",
                        "safETH"
                    )
                )
            )
        );  

        // RocketPool derivative deploy and configue
        reth = Reth(
            payable(
                new TransparentUpgradeableProxy(
                    address(new Reth()),
                    admin,
                    abi.encodeWithSelector(
                        Reth.initialize.selector,
                        address(safEth)
                    )
                )
            )
        );  
        safEth.addDerivative(address(reth), DEFAULT_DERIVATIVE_WEIGHT);
        safEth.setMaxSlippage(0, DEFAULT_MAX_SLIPPAGE); 
        derivatives[0] = IDerivative(reth);

        // Frax derivative deploy and configue
        sfrxEth = SfrxEth(
            payable(
                new TransparentUpgradeableProxy(
                    address(new SfrxEth()),
                    admin,
                    abi.encodeWithSelector(
                        SfrxEth.initialize.selector,
                        address(safEth)
                    )
                )
            )
        );  
        safEth.addDerivative(address(sfrxEth), DEFAULT_DERIVATIVE_WEIGHT);
        safEth.setMaxSlippage(1, DEFAULT_MAX_SLIPPAGE);
        derivatives[1] = IDerivative(sfrxEth);

        // Lido derivative deploy and configue
        wstEth = WstEth(
            payable(
                new TransparentUpgradeableProxy(
                    address(new WstEth()),
                    admin,
                    abi.encodeWithSelector(
                        WstEth.initialize.selector,
                        address(safEth)
                    )
                )
            )
        );  
        safEth.addDerivative(address(wstEth), DEFAULT_DERIVATIVE_WEIGHT);
        safEth.setMaxSlippage(2, DEFAULT_MAX_SLIPPAGE); 
        derivatives[2] = IDerivative(wstEth);
    }

    function testFailUnstake() public {
        // Add Ether to the current contract's balance
        vm.deal(address(this), TEST_DEPOSIT_WEI);
        // Stake Ether in the safEth contract will always fail
        safEth.stake{value: TEST_DEPOSIT_WEI}();

        // Unstake all Ether
        safEth.unstake(safEth.balanceOf(address(this)));

        // Ether value after unstake whould be within 1% of slippage loss
        assertTrue(within1Percent(address(this).balance, TEST_DEPOSIT_WEI));
    }

    // Check if ratio between 2 amounts is less than 1%
    function within1Percent(uint post, uint prev) private pure returns (bool) {
        return getDifferenceRatio(post, prev) > 100;
    }

    // Get ratio between 2 amounts such that % diff = 1/ratio
    // Example: 200 = 0.5%, 100 = 1%, 50 = 2%, 25 = 4%, etc
    // Useful for comparing ethers bignumbers that dont support floating point numbers
    function getDifferenceRatio(uint a, uint b) private pure returns (uint ratio) {
        if(a == b) {
            ratio = type(uint).max;
        } else {
            ratio = a / (a > b ? a - b : b - a);    
        }
    }
}

The result will be a passed test with an error Assertion Failed. That means the user gets back less Ether than expected.

Running 1 test for test/foundry/SafEth.t.sol:SafEthTest
[PASS] testFailUnstake() (gas: 878056)
Logs:
  Error: Assertion Failed

Test result: ok. 1 passed; 0 failed; finished in 25.84s

Tools Used

VSCodium, Foundry

Recommended Mitigation Steps

One possible solution is to check the difference in Ether balance for each derivative within the unstake() function. This can be achieved by comparing the contract's Ether balance before and after calling the withdraw() function of the derivative. If the difference is zero, the contract should revert the transaction, preventing the burning of safEth tokens. Modify the unstake() function as follows:

/**
    @notice - Unstake your safETH into ETH
    @dev - unstakes a percentage of safEth based on its total value
    @param _safEthAmount - amount of safETH to unstake into ETH
*/
function unstake(uint256 _safEthAmount) external {
    require(pauseUnstaking == false, "unstaking is paused");
    uint256 safEthTotalSupply = totalSupply();
    uint256 ethAmountBefore = address(this).balance;

    for (uint256 i = 0; i < derivativeCount; i++) {
        // withdraw a percentage of each asset based on the amount of safETH
        uint256 derivativeAmount = (derivatives[i].balance() *
            _safEthAmount) / safEthTotalSupply;
        if (derivativeAmount == 0) continue; // if derivative empty ignore

        // Add check for a zero Ether received
        uint256 ethBefore = address(this).balance;
        derivatives[i].withdraw(derivativeAmount);
        require(address(this).balance - ethBefore != 0, "Receive zero Ether");
    }
    // ...
}

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

toshiSat marked the issue as sponsor confirmed

[c4-sponsor]

toshiSat marked the issue as disagree with severity

[Picodes]

This finding is basically saying: "By commenting out the line responsible for sending Ether back" no ether is sent back. How can this happen in production?

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid