Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitigation Confirmed for Mitigation of H-08: Issue mitigated with error #35

Open
code423n4 opened this issue May 8, 2023 · 2 comments
Labels
mitigation-confirmed MR-H-08 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Mitigated issue

H-08: Staking, unstaking and rebalanceToWeight can be sandwiched (Mainly rETH deposit).

The issue was a MEV sandwich attack on withdraw() (called by unstake()). In Reth simply because slippage protection was missing, and in SfrxEth because the slippage was evaluated within the transaction rather than when it was sent.
The latter case is exploitable if the price used to evaluate the slippage can be manipulated. In SfrxEth FrxEthEthPool.price_oracle() was used (through ethPerDerivative()), which is a time-weighted exponential moving average.
In WstEth a 1-1 peg was assumed between stETH and wstETH in the slippage evaluation, so it couldn't be manipulated. Note that this has been changed in the mitigation of H-06: WstEth derivative assumes a ~1=1 peg of stETH to ETH..

Mitigation review

Execution independent slippage protection has been introduced in unstake() by the mitigation of M-12: No slippage protection on stake() in SafEth.sol. This solves the problem for Reth. (The spot price oracle has also been replaced by a Chainlink price feed, but the issue is mitigated by the new slippage protection alone.)

The 1-1 peg assumption in WstEth has been replaced by a Chainlink price feed, which should be stable and secure enough.

There is no Chainlink price feed for frxEth so SfrxEth.ethPerDerivative() hardcodes a 1-1 peg with ETH, so there is nothing to manipulate (just like previously was the case with WstEth).

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
Copy link

elmutt commented May 8, 2023

thanks

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 11, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@Picodes Picodes added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value unmitigated mitigation-confirmed and removed MR-Mitigation of H-08: Issue mitigated with error mitigation-confirmed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value unmitigated labels May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mitigation-confirmed MR-H-08 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants