ERC4626RouterBase.withdraw should use a **max** shares out check

[code423n4]

Lines of code

https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/ERC4626RouterBase.sol#L45

Vulnerability details

Impact

The docs/video say ERC4626RouterBase.sol is in scope as its part of TurboRouter

The ERC4626RouterBase.withdraw function withdraws the asset amount parameter by burning shares.

function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 minSharesOut
) public payable virtual override returns (uint256 sharesOut) {
    // @audit-info from = msg.sender
    if ((sharesOut = vault.withdraw(amount, to, msg.sender)) < minSharesOut) {
        revert MinAmountError();
    }
}

It then checks that the burned shares sharesOut are not less than a minSharesOut amount.
However, the user wants to be protected against burning too many shares for their specified amount, and therefore a maxSharesBurned amount parameter should be used.

The user can lose their entire shares due to the wrong check.

this extends to TurboRouter.withdraw

POC

User calls Router.withdraw(amount=1100, minSharesOut=1000) to protect against not burning more than 1000 shares for their 1100 asset amount.
However, there's an exploit in the vault which makes the sharesOut = 100_000, the entire user's shares.
The check then passes as it only reverts if 100_000 < 1000.

Recommended Mitigation Steps

function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
-    uint256 minSharesOut
+    uint256 maxSharesIn
) public payable virtual override returns (uint256 sharesOut) {
-    if ((sharesOut = vault.withdraw(amount, to, msg.sender)) < minSharesOut) {
+    if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
        revert MinAmountError();
    }
}

Also, rename the variable in TurboRouter.withdraw.

[Joeysantoro]

This should be a medium severity issue. The function logic is correct, its just not a useful check in its current state.

[Joeysantoro]

ERC4626-Alliance/ERC4626-Contracts#9

[GalloDaSballo]

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