Mitigation Confirmed for H-07

[code423n4]

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of H-07: Mitigation error, see comments

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

Comments

The issue described in H-07 is about a missing validation while burning RETH for ETH in the withdraw() function of the Reth derivative.

Technical Details

The proposed mitigation introduces a canWithdrawFromRocketPool() function to add the check to see if there is enough liquidity to call burn():

https://github.com/asymmetryfinance/smart-contracts/pull/258/files#diff-6abc8f2e4ad1647a12784e9fbf18e9c5f86c05668e3e89e2a51ab569992b214fR92-R108

function canWithdrawFromRocketPool(
    uint256 _amount
) private view returns (bool) {
    address rocketDepositPoolAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked("contract.address", "rocketDepositPool")
            )
        );
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
            rocketDepositPoolAddress
        );
    uint256 _ethAmount = RocketTokenRETHInterface(rethAddress())
        .getEthValue(_amount);
    return rocketDepositPool.getExcessBalance() >= _ethAmount;
}

As we can see in the previous snippet, the function uses getExcessBalance() to implement the check, which is not accurate, as the condition should not only check for the excess balance but also for the available ETH balance in the contract. This can be seen in the implementation of getTotalCollateral():

https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/token/RocketTokenRETH.sol#L72C1-L75

function getTotalCollateral() override public view returns (uint256) {
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(getContractAddress("rocketDepositPool"));
    return rocketDepositPool.getExcessBalance().add(address(this).balance);
}

The pull request also adds RocketSwapRouter as a fallback mechanism if the collateral amount is not enough. However, RocketSwapRouter already considers burning RETH through the Rocket Pool protocol as part of its internal implementation (see implementation of swapFrom() here https://etherscan.deth.net/address/0x16D5A408e807db8eF7c578279BEeEe6b228f1c1C#code). This means that RocketSwapRouter could be used directly to provide a more optimal experience, as the implementation will try to first burn RETH and swap the rest using Uniswap or Balancer, minimizing slippage.

Recommendation

Use directly the swapFrom() function of RocketSwapRouter, which will correctly use the burn mechanism of RETH, and swap the rest if the collateral doesn't cover the full amount. This will not only solve the issues, but also provide a more optimal solution.

Out of scope

Low: note that RocketSwapRouter swapFrom() function will send ETH back to the contract, there is no need to unwrap the WETH:

https://github.com/asymmetryfinance/smart-contracts/pull/258/files#diff-6abc8f2e4ad1647a12784e9fbf18e9c5f86c05668e3e89e2a51ab569992b214fR140

[elmutt]

thanks

[d3e4]

rocketDepositPool.getExcessBalance() >= _ethAmount is a tighter constraint which is required in the later call to withdrawDepositCollateral(ethAmount). This is detailed in the original report.

[romeroadrian]

rocketDepositPool.getExcessBalance() >= _ethAmount is a tighter constraint which is required in the later call to withdrawDepositCollateral(ethAmount). This is detailed in the original report.

Correct, it is tighter, which ends up being more restrictive than needed. Could you please double check this?

The burn() function considers the available ETH balance in RocketTokenRETH, plus any excess available in the deposit pool. This can be seen in the linked function:

function withdrawDepositCollateral(uint256 _ethRequired) private {
    // Check rETH contract balance
    uint256 ethBalance = address(this).balance;
    if (ethBalance >= _ethRequired) { return; }
    // Withdraw
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(getContractAddress("rocketDepositPool"));
    rocketDepositPool.withdrawExcessBalance(_ethRequired.sub(ethBalance));
}

withdrawExcessBalance() is called with the amount minus the ETH balance of the contract.

So if RocketTokenRETH has 1 ETH and the deposit pool has 5 ETH in excess, and we want to burn 5.5 ETH, then rocketDepositPool.getExcessBalance() >= 5.5 ether will return false, but it should be possible as we can use the 1 ETH in the contract and withdraw 4.5 from the deposit pool. Which is exactly what getTotalCollateral() checks.

[d3e4]

@romeroadrian You are right after all. And you'd better be, because otherwise burn() itself would have the wrong require, which checks precisely that getTotalCollateral() >= ethAmount! However, since getExcessBalance() <= getTotalCollateral() the issue is still mitigated, but suboptimally. It doesn't lead to errors, but should indeed have been return RocketTokenRETHInterface(rethAddress()).getTotalCollateral() >= _ethAmount;. But as we both suggest (here and in #56) using only the RocketSwapRouter is an even better solution.

[c4-judge]

Picodes marked the issue as satisfactory