Mitigation Confirmed for M-02

[code423n4]

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of M-02: Issue not mitigated

Link to Issue: https://github.com/code-423n4/2023-03-asymmetry-findings/issues/1049

Comment

Issue M-02 describes an edge case in which the SfrxEth derivative may revert under an scenario where the calculation of the redeem amount in the sfrxETH vault is zero, causing a potential DoS in the protocol.

The proposed fix by the sponsor is to use the derivative enable/disable feature, probably with the intention of disabling the SfrxEth in case the issue is manifested. I don't think this fully mitigates the issue for at least two major reasons, which are described below.

Technical Details

The reasoning for the issue in the original report is that if the derivative weight has been set to 0, then a series of staking/unstaking actions could dilute the amount for the SftxEth derivative and eventually lead to the withdraw() being called with 1.

The proposed mitigation misses a very important detail, which is that the disable feature operates on a global level. While the issue may be experienced by a few users that triggered the particular conditions for this to happen, other users may still have a normal or significant amount of funds in the derivative. The protocol cannot simply disable the derivative just to address the problem of a few, doing so may also lock funds for other users of the protocol.

It should be also noted that the described scenario in the original issue does not represent the only case which may trigger the issue. Any situation where the calculated redeem amount is low enough to be converted to zero assets given the required conditions (total assets and total shares) in the sfrxETH vault will trigger the issue. As a quick example, the following test reproduces the case where the user has a low number of SafEth tokens, note that here the weight of the derivative isn't set to zero:

// Test for mitigation contest. MR-M-02
function test_SafEth_SfrxRedeemDos() public {
    address sfrxEthVault = sfrxEth.SFRX_ETH_ADDRESS();
    uint256 totalShares = ISfrxEthVault(sfrxEthVault).totalSupply();

    // Ensure assets is below shares to match conditions in original issue
    vm.store(sfrxEthVault, bytes32(uint256(7)), bytes32(totalShares - 100e18));

    // Setup derivative
    vm.prank(deployer);
    safEth.addDerivative(address(sfrxEth), 1);

    // user has 1 ether
    uint256 initialAmount = 1 ether;
    vm.deal(user, initialAmount);

    // user stakes ether
    vm.prank(user);
    safEth.stake{value: initialAmount}();

    uint256 userShares = safEth.balanceOf(user);

    // user withdraws everything but 1 share
    vm.prank(user);
    safEth.unstake(userShares - 1);

    // Now user tries to withdraw the remaining share. This will fail on the redeem function due
    // to the same issue described in the original issue.
    vm.prank(user);
    safEth.unstake(1);
}

This also demonstrates that the proposed solution isn't a proper mitigation. The protocol can't shutdown a derivative if an individual user is faced with such conditions that trigger the issue.

Recommendation

Apply the recommendation proposed in the original report for M-02. Use the previewRedeem() function to check if the returned amount will be zero to skip the revert. This will address the issue at the individual level of a particular account, without dealing with other potential issues of disabling a derivative.

[elmutt]

that makes sense thanks

[c4-judge]

Picodes marked the issue as satisfactory