Mitigation Confirmed for Mitigation of H-01: Issue mitigated

[code423n4]

Mitigated issue

H-01: An attacker can manipulate the preDepositvePrice to steal from other users.

The issue was that the underlyingValue to totalSupply ratio in SafEth.sol could be severely inflated to cause significant rounding errors in favour of the first depositor (a standard inflation attack). underlyingValue is based on the balances of the derivatives, which was determined directly by the balanceOf() the derivative. The vulnerability was that the return value of balanceOf() can be manipulated by transferring tokens directly to the derivative.

Mitigation review

The mitigation removes the reliance on balanceOf() and instead keeps an internal accounting of the balance in each derivative in the variable underlyingBalance. In each derivative balance() now returns underlyingBalance instead of using balanceOf(), and underlyingBalance is increased in deposit() by the change in token balance after the conversion of msg.value in its respective pool, and decreased in withdraw() by the amount withdrawn. Since deposit() and withdraw() are only callable by SafEth via its stake() and unstake() the underlyingValue can only be changed through the legitimate paths such that all changes to underlyingValue are also reflected in totalSupply.

Potential consequences

Since the accounting of tokens is now detached from the true token balance given by balanceOf(), if tokens after all are transferred directly to the derivatives they will be inaccessible. Since tokens are not supposed to be transferred to the derivatives this should not be an issue. And since withdrawals in the derivative now only sends the new ether received rather than its total balance (which it did previously) the same applies to ether.

Insufficient test

The test added doesn't truly confirm that the inflation attack is unsuccessful. It only tests that the second deposit increases the supply of shares, but does not confirm that the rounding errors are small. It should rather test that withdrawing the second deposit returns approximately (either more or less!) the same amount as was deposited. The test may fail to identify a possible inflation attack if, for example, the shares uses a different number of decimals compared to the underlying balance, or if virtual shares are used.

[elmutt]

thanks

[c4-judge]

Picodes marked the issue as satisfactory