search

code-423n4 / 2023-05-asymmetry-mitigation-findings

2 stars 2 forks source link

Mitigation Confirmed for Mitigation of H-04: Issue mitigated #32

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Mitigated issue

H-04: Price of sfrxEth derivative is calculated incorrectly.

The issue was a simple arithmetical error in SfrxEth.ethPerDerivative() where (frxETH/sfrxETH) / (ETH/frxETH) was calculated instead of the correct (frxETH/sfrxETH) * (ETH/frxETH) = ETH/sfrxETH.

Mitigation review

The ETH/frxETH price from FrxEthEthPool.price_oracle() has been replaced by the identity, i.e. a perfect peg is assumed in the calculation, hence the issue vanishes.

However, a protection against the invalidity of this assumption is put in place: FrxEthEthPool.price_oracle() is still consulted and if its price deviates more than 0.1% from 1e18 it is assumed that frxETH has depegged and SfrxEth.ethPerDerivative() reverts. frxETH has a very strong peg. It has never been outside of the range 0.996-1.001. It therefore seems reasonable to assume that a 0.1% deviation is indeed a depegging event.

We have to consider a natural depegging and a malicious depegging caused by manipulation.

In the event of a natural depegging users might want to protect their funds by withdrawing their sfrxETH, however they can only do so by unstaking safETH. This would withdraw funds from all other derivatives as well, which is not desirable. Therefore, it seems the only solution in this event is for the owner of SafEth to withdraw the sfrxETH by adjusting the weights and calling rebalanceToWeights(). Since users explicitly hand over their funds to be managed by SafEth, this makes perfect sense. The SafEth protocol would then have to monitor frxETH and be ready to act on a depegging.

As for a malicious depegging note that price_oracle() is an exponential moving average. As such the effect of a one time (flashloan) manipulation will persist for some time. This means that even though the depegging has already been restored the 0.1% check may cause SfrxEth.ethPerDerivative() to revert for some time thereafter. However, it seems that this DoS of stake() should be very brief, and an acceptable downside of this precautionary measure.

Since execution independent slippage protection has been introduced in stake()and unstake() by the mitigation of M-12: No slippage protection on stake() in SafEth.sol all issues based on oracle manipulation are mitigated.

elmutt commented 1 year ago

thanks

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory