Mitigation of M-12: mitigation error, see comments

[code423n4]

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of M-12: mitigation error, see comments

Link to Issue: code-423n4/2023-03-asymmetry-findings#150

Comments

While the proposed change correctly mitigates the issue, in the sense that it introduces a user controlled slippage for stake() and unstake(), the new changes conflict with an existing implementation that should be addressed.

Technical Details

The changeset adds a minimum output amount for the stake() and unstake() functions present in the SafEth.sol contract. If the output amount is below this limit, the transaction gets reverted, letting the user be in control of the final amounts they will receive.

This new feature is an excellent addition to the protocol. However, it conflicts with the previous slippage control, present in each of the 3 derivatives:

• https://github.com/asymmetryfinance/smart-contracts/blob/minouts2/contracts/SafEth/derivatives/Reth.sol#L173-L174
• https://github.com/asymmetryfinance/smart-contracts/blob/minouts2/contracts/SafEth/derivatives/WstEth.sol#L60
• https://github.com/asymmetryfinance/smart-contracts/blob/minouts2/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

This means that a user may control slippage only under the (currently) 1% margin imposed in each derivative, as setting a lower amount will eventually be overridden by the limit on each derivative, which will revert the operation.

During times of high market volatility, a user may want to exit their position and set a high slippage, and eventually take losses beyond the protocol defined slippage present in each of the derivatives.

Recommendation

My recommendation is to keep the current changes in the pull request and remove the maxSlippage in each derivative. If the intention is to also have a finer control over the slippage in each individual derivative, then extend the new _minOut parameter to be one per derivative. This would allow the user to have complete control over their position. The protocol can provide safe defaults using off-chain mechanisms when the user operates through a front-end application.

[elmutt]

Your issue makes sense but we awe are ok with having slippage in the derivatives as well as the main contract.

[d3e4]

This review doesn't explicitly mention the new issue reported in #67. But the recommendation here is essentially the same as in #67 and solves that issue as well.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #67