Open code423n4 opened 2 years ago
This should be a medium severity issue. The function logic is correct, its just not a useful check in its current state.
I agree with both sides, ultimately the check doesn't provide protection against loss of value, as such medium severity is appropriate.
The sponsor has mitigated in a subsequent PR
Lines of code
https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/ERC4626RouterBase.sol#L45
Vulnerability details
Impact
The
ERC4626RouterBase.withdraw
function withdraws the assetamount
parameter by burningshares
.It then checks that the burned shares
sharesOut
are not less than aminSharesOut
amount. However, the user wants to be protected against burning too many shares for their specifiedamount
, and therefore amaxSharesBurned
amount parameter should be used.The user can lose their entire shares due to the wrong check.
POC
User calls
Router.withdraw(amount=1100, minSharesOut=1000)
to protect against not burning more than1000
shares for their1100
asset amount. However, there's an exploit in the vault which makes thesharesOut = 100_000
, the entire user's shares. The check then passes as it only reverts if100_000 < 1000
.Recommended Mitigation Steps
Also, rename the variable in
TurboRouter.withdraw
.