Mitigation Confirmed for Mitigation of H-02: Issue mitigated with error

[code423n4]

Mitigated issue

H-02: A temporary issue shows in the staking functionality which leads to the users receiving less minted tokens.

The issue was that stake() calls Reth.ethPerDerivative() twice but modifies the Rocket Pool balance in between those two calls. Reth.ethPerDerivative() returns either of two oracle prices, Rocket Pool or Uniswap, depending on the Rocket Pool balance which determines if it is possible to deposit there. These two oracles do not in general return the same value. Thus Reth.ethPerDerivative() might return inconsistent values within stake().

Mitigation review

The inconsistency between the two calls to Reth.ethPerDerivative() is eliminated by modifying Reth.ethPerDerivative() to instead only return a single oracle price: the Chainlink rETH/ETH price feed. However, they may still be inconsistent with Reth.deposit() which converts ETH to rETH. That is, Reth.ethPerDerivative() should be the inverse of Reth.deposit(). But Reth.deposit() now swaps ETH into rETH using Balancer. Just like Rocket Pool and Uniswap can have different exchange rates, which gave rise to H-02, so may Balancer have a different rate than the one returned by the Chainlink rETH/ETH price feed. Thus the rETH/ETH rate is still inconsistently determined.

What we really want is that staking doesn't diminish the value of unstaking, i.e. if Alice can unstake her shares for X ETH before Bob stakes, she should also get X ETH if she unstakes the same shares immediately after Bob stakes (up to changes in pool exchange rates due to a change in supply). Therefore totalStakeValueEth must be the very same value in ETH as we would get if we withdrew the newly deposited amounts in each derivative. Chainlink does not provide this exact value.

However, there might be no viable alternative. Using the more accurate spot price from RocketTokenRETH.getEthValue() open up for manipulations, reintroducing issue H-08.

This is issue is actually more general, and is reported as a new issue titled "Inaccurate ethPerDerivative() may cause a loss for holders of safETH when derivative distribution in unstable".

Conclusion

ethPerDerivative() should be as accurate as possible, but there is a trade-off between accuracy and manipulability. It seems that as far as this issue goes, it is siffuciently mitigated. For what may remain an issue, see ""Inaccurate ethPerDerivative() may cause a loss for holders of safETH when derivative distribution in unstable".

Mitigation error: No sanity check on Chainlink response

There are no sanity checks on the Chainlink price feed return data, especially that it is not stale. Here is an overview on this matter. See the error report on this titled "[H-02, H-05, H-06, H-08] mitigation error: No sanity check on Chainlink price feed".

[elmutt]

thanks

[c4-judge]

Picodes marked the issue as satisfactory