Reappearance of M-02 in `SafEth.unstake()`

[code423n4]

Reappearance of M-02 in SafEth.unstake()

https://github.com/asymmetryfinance/smart-contracts/blob/ec582149ae9733eed6b11089cd92ca72ee5425d6/contracts/SafEth/SafEth.sol#L148-L151

Description

The changes in SafEth.unstake() has introduced a new issue parallel to the one present in SfrxEth.withdraw() which was reported in M-02: sFrxEth may revert on redeeming non-zero amount, i.e. SafEth.unstake() may revert as a consequence of a valid call to a derivative's withdraw(). 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. Most of what is said there apply to this new issue as well. The only difference here is that in M-02 the revert happens in withdraw(), whereas in this new issue the revert is simply deferred to unstake(), immediately after the call to withdraw(). Otherwise the issue is similar, i.e. withdraw(derivativeAmount) may send zero ether for non-zero derivativeAmount, which then triggers a revert.

Proof of Concept

The foor-loop in SafEth.unstake() is now

for (uint256 i = 0; i < count; i++) {
    if (!derivatives[i].enabled) continue;
    // withdraw a percentage of each asset based on the amount of safETH
    uint256 derivativeAmount = (derivatives[i].derivative.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].derivative.withdraw(derivativeAmount);
    require(
        address(this).balance - ethBefore != 0,
        "Receive zero Ether"
    );
}

withdraw(1) may send zero ether because of rounding. For an explicit proof of this see my report titled "Reappearance of M-02 in WstEth.withdraw()" which demonstrates a case identical to M-02 in WstEth.withdraw(). If that issue is fixed such that WstEth.withdraw(1) doesn't revert, then it will send zero ether. It is fair to assume that this may happen for any derivative.

The new culprit is the check require(address(this).balance - ethBefore != 0, "Receive zero Ether");.

Recommended Mitigation Steps

There is a check in each derivative that the ether transfer is successful. Therefore this check seems unnecessary in unstake(). If a check is deemed necessary it should only be performed if the derivativeAmount is large enough (i.e. greater than a very small amount, perhaps just 1).

[toshiSat]

When derivative is disabled it won't get to this line. It will be good to pinpoint derivatives that are misbehaving and having the check to make sure ETH is sent back feels better

[romeroadrian]

@d3e4 I noticed this in the development branch, but I think the following lines are not in the scope of any pull requests:

require(
        address(this).balance - ethBefore != 0,
        "Receive zero Ether"
    );

This is the commit for the change https://github.com/asymmetryfinance/smart-contracts/commit/615db430b6c6546715983ff48e9af453abbd9e2e

[d3e4]

When derivative is disabled it won't get to this line. It will be good to pinpoint derivatives that are misbehaving and having the check to make sure ETH is sent back feels better

See the MRs #15, #43 and #63 of M-02 for why disabling the derivative isn't a full solution. It is redundant to check that ETH is sent for each derivative by itself in unstake(). There is already such a check in each derivative. If you want a check in unstake() this should then be after the for-loop that ethAmountToWithdraw > 0 but it is already done even better by checking that ethAmountToWithdraw > _minOut.

@d3e4 I noticed this in the development branch, but I think the following lines are not in the scope of any pull requests:

require(
        address(this).balance - ethBefore != 0,
        "Receive zero Ether"
    );

This is the commit for the change asymmetryfinance/smart-contracts@615db43

It is actually there in the PR for this issue: enable / disable derivatives

[romeroadrian]

@d3e4 I noticed this in the development branch, but I think the following lines are not in the scope of any pull requests:

require(
        address(this).balance - ethBefore != 0,
        "Receive zero Ether"
    );

This is the commit for the change asymmetryfinance/smart-contracts@615db43

It is actually there in the PR for this issue: enable / disable derivatives

I see, yeah this was one of the raised concerns in the discord chat. The sponsor branched fixes from already modified parts of the code. If you see the list of commits for that PR (here https://github.com/asymmetryfinance/smart-contracts/pull/264/commits) there's no indication of that specific commit (615db43) being included in the changeset. Therefore I think it should be OOS.

[c4-judge]

Picodes marked the issue as duplicate of #70