The router does not have the intended behavior: according to the EIP https://eips.ethereum.org/EIPS/eip-4626, withdraw burn shares to withdraw exactly assets, so the slippage protection should protect the user from burning too many shares.
Furthermore, comments in the interface are incorrect as the user does not receive any share.
Recommended Mitigation Steps
Instead of minSharesOut it should be maxSharesIn.
Instead of
if ((sharesOut = vault.withdraw(amount, to, msg.sender)) < minSharesOut) { revert MinAmountError(); }
it should be:
if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) { revert <Error>(); }
Joeysantoro
commented
2 years ago
Lines of code
https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/ERC4626RouterBase.sol#L47 https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/interfaces/IERC4626RouterBase.sol#L69
Vulnerability details
Impact
The router does not have the intended behavior: according to the EIP https://eips.ethereum.org/EIPS/eip-4626,
withdrawburn shares to withdraw exactlyassets, so the slippage protection should protect the user from burning too many shares.Furthermore, comments in the interface are incorrect as the user does not receive any share.
Recommended Mitigation Steps
Instead of
minSharesOutit should bemaxSharesIn.Instead of
if ((sharesOut = vault.withdraw(amount, to, msg.sender)) < minSharesOut) { revert MinAmountError(); }it should be:if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) { revert <Error>(); }