Introduced change in stake() may affect calculation of minted shares

[code423n4]

NEW ISSUE - MITIGATION IS NOT CONFIRMED

NEW ISSUE - MITIGATION IS NOT CONFIRMED

[adriro-NEW-M-02] Introduced change in stake() may affect calculation of minted shares

Link to changeset: https://github.com/asymmetryfinance/smart-contracts/pull/209/files

Impact

Pull request #209 (link above) introduces a change in how the preDepositPrice is calculated. Now, if the underlyingValue is zero, the preDepositPrice will be calculated as 1e18, similar to the case when totalSupply == 0:

https://github.com/asymmetryfinance/smart-contracts/pull/209/files#diff-badfabc2bc0d1b9ef5dbef737cd03dc2f570f6fd2074aea9514da9db2fff6e4eR78

uint256 preDepositPrice; // Price of safETH in regards to ETH
if (totalSupply == 0 || underlyingValue == 0)
    preDepositPrice = 10 ** 18; // initializes with a price of 1
else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

Before this change, if underlyingValue == 0, then preDepositPrice == 0 and eventually the function will revert as the calculation of mintAmount divides by preDepositPrice. With this new change, it is possible to have a situation where totalSupply != 0 but underlyingValue == 0, which will cause preDepositPrice == 1e18.

This should be considered an edge case, however it should be noted that with the introduction of disabled derivatives, it might be the case that derivatives are disabled, causing the underlyingValue variable to be zero. Given this scenario, mintAmount will be equal to totalStakeValueEth, without any restrictions to totalSupply (meaning there may be existing shareholders of SafEth):

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
                                ||
                                \/
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / 1e18;
                                ||
                                \/
uint256 mintAmount = totalStakeValueEth;

Recommendation

Revert the highlighted change.

[toshiSat]

I feel like either way there will be an edge case surrounding this bit of code. If the underlying value is 0 and total supply > 0 then all derivatives will be disabled. If that's the case, stake would not execute

[d3e4]

The effect of this would be that the staker would be forced to share the value of his stake with the already existing shares. But given the nature of the derivatives I would argue that it is impossible that the underlying value would drop to zero without implying a complete failure of that derivative token. Therefore the issue is moot in this protocol. But it's an interesting idea, which I believe was a valid issue in a recent contest, where the context was different.

[d3e4]

For underlyingValue to be 0 each derivative must be disabled OR have zero balance OR its token be worthless. Any derivative which doesn't fulfil this condition contributes positively to underlyingValue, breaking it's zero value. !derivative.enabled || derivative.balance() == 0 || derivative.ethPerDerivative() == 0

For totalStakeValueEth to be > 0 at least one derivative must be enabled AND have non-zero weight AND its token be worth something. The derivatives which don't fulfil this do not contribute to totalStakeValueEth and therefore don't count. derivative.enabled && derivative.weight > 0 && derivative.ethPerDerivative() > 0

We are looking for the case where both of the above hold true. Note how the second condition contradicts all but one of the options in the first condition. Therefore, any derivative that might contribute to totalStakeValueEth while not contributing to underlyingValue must fulfil derivative.balance() == 0. This is just - or equivalent to - having newly added derivatives.

We can thus also see that the specific claim that "it might be the case that derivatives are disabled, causing the underlyingValue variable to be zero" is incorrect. If they are disabled they will not contribute to totalStakeValueEth.

A question then is, can we have previously added and deposited-into derivatives which have been brought out of action, still holding funds? Yes, there is nothing that technically prevents this. Derivatives can be arbitrarily disabled and deweighted. Weights control staking, disabling controls both staking and unstaking. But there is equally no reason the owner would arbitrarily do this.

We don't even need underlyingValue == 0 for the disabling of derivatives to pose a problem. When a derivative with funds is disabled it is neither staked into nor unstaked from. This means that it is as if those funds simply disappeared. This causes holders to lose value, if they unstake. New stakers would neither gain nor lose, but if the disabled derivative is reenabled, they will suddenly be able to claim their share of these new funds as well, at the expense of the previous stakers. underlyingValue == 0 only adds the minor complication that new stakers also may lose or gain.

A derivative would only be disabled if it doesn't work properly. That would mean it is uncertain whether its funds will be lost or not. So we cannot pretend either. A decision will have to be made by the owner on a case by case basis on how to handle this.

Technically this report fails to show how the problematic conditions might arise. But disregarding that, the protocol is not at risk because only the owners could cause these conditions, and there is no reason to believe they would. Therefore I think this issue is Low risk.

[liveactionllama]

Per discussion with the judge, marking this as QA. Also, see their comment here for more detail.

Since QA are not eligible for awards, nullifying.