Reappearance of M-02 in `WstEth.withdraw()`

[code423n4]

Reappearance of M-02 in WstEth.withdraw()

https://github.com/asymmetryfinance/smart-contracts/blob/ec582149ae9733eed6b11089cd92ca72ee5425d6/contracts/SafEth/derivatives/WstEth.sol#L71-L72

Description

The changes in WstEth.withdraw() has introduced a new issue exactly parallel to the one present in SfrxEth.withdraw() which was reported in M-02: sFrxEth may revert on redeeming non-zero amount, i.e. WstEth.withdraw(_amount) may revert when _amount > 0. For why this is an issue please refer to M-02. The mitigation of M-02 was to enable/disable derivatives. See my mitigation review of M-02 for how that issue is not resolved and why I think the mitigation may be insufficient. What is said there equally apply, mutatis mutandis, to this new issue.

Proof of Concept

WstEth.withdraw() now begins

uint256 stEthAmount = IWStETH(WST_ETH).unwrap(_amount);
require(stEthAmount > 0, "No stETH to unwrap");

We therefore have the same problem as in M-02 if IWStETH(WST_ETH).unwrap(1) == 0. WstEth.unwrap() is

function unwrap(uint256 _wstETHAmount) external returns (uint256) {
    require(_wstETHAmount > 0, "wstETH: zero amount unwrap not allowed");
    uint256 stETHAmount = stETH.getPooledEthByShares(_wstETHAmount);
    _burn(msg.sender, _wstETHAmount);
    stETH.transfer(msg.sender, stETHAmount);
    return stETHAmount;
}

We then ask whether stETH.getPooledEthByShares(1) == 0. StETH.getPooledEthByShares() is:

function getPooledEthByShares(uint256 _sharesAmount) public view returns (uint256) {
    uint256 totalShares = _getTotalShares();
    if (totalShares == 0) {
        return 0;
    } else {
        return _sharesAmount
            .mul(_getTotalPooledEther())
            .div(totalShares);
    }
}

So just like in M-02, if _getTotalPooledEther() < totalShares then IWStETH(WST_ETH).unwrap(1) == 0 and WstEth.withdraw(1) reverts.

Recommended Mitigation Steps

Replace require(stEthAmount > 0, "No stETH to unwrap"); with if (stEthAmount > 0) return;

[toshiSat]

in our remove derivative pr we never call withdraw for a derivative contract that's disabled.

[romeroadrian]

I thought of this case given the very good finding in M-02 and concluded that is not a valid scenario, realistically there's no way that a positive amount of wstETH unwraps as zero amount of stETH. I think a low severity would be more appropriate.

[d3e4]

in our remove derivative pr we never call withdraw for a derivative contract that's disabled.

See the MRs #15, #43 and #63 of M-02 for why disabling the derivative isn't a full solution.

I thought of this case given the very good finding in M-02 and concluded that is not a valid scenario, realistically there's no way that a positive amount of wstETH unwraps as zero amount of stETH. I think a low severity would be more appropriate.

I agree that it seems unlikely that IWStETH(WST_ETH).unwrap(1) == 0 but I cannot see that it would be impossible. It's also easy enough to fix.

[romeroadrian]

I agree that it seems unlikely that IWStETH(WST_ETH).unwrap(1) == 0 but I cannot see that it would be impossible. It's also easy enough to fix.

wstETH is a wrapper around stETH which is a rebasing token. If you wrap 1 token then that will be always be unwrapped as at least 1 stETH, because stETH being a rebasing token it will be receiving ETH from staking rewards. This means that:

1. Minting stETH increases shares but also increases the ETH as the user needs to submit the ETH.
2. When staking rewards are distributed in the contract ETH is only increased, not shares.

I can't see how _getTotalPooledEther() < totalShares would hold.

[Picodes]

@romeroadrian it is very unlikely but not impossible if for example staked ETH backing stETH are slashed, no? In this case _getTotalPooledEther would decrease.

[romeroadrian]

@romeroadrian it is very unlikely but not impossible if for example staked ETH backing stETH are slashed, no? In this case _getTotalPooledEther would decrease.

That's an interesting point!

I wasn't referring to impossible (as something catastrophic may happen to Lido, in which case an admin action in SafEth would be more appropriate), but honestly I wasn't considering slashing. That sounds a bit more likely.

I'm ok then, given this reasoning, thanks for pointing this out.

[c4-judge]

Picodes marked the issue as primary issue