QA Report

[code423n4]

Low and non-critical bugs

wrong implementation of ERC4626RouterBase's withdraw function

In the interface we can see the function description:

/** 
 @notice withdraw `amount` from an ERC4626 vault.
 @param vault The ERC4626 vault to withdraw assets from.
 @param to The destination of assets.
 @param amount The amount of assets to withdraw from vault.
 @param minSharesOut The min amount of shares received by `to`.
 @return sharesOut the amount of shares received by `to`.
 @dev throws MinOutError   
*/
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 minSharesOut
) external payable returns (uint256 sharesOut);

As you probably know, the ERC4626 withdraw function recieves 3 parameters - from, to and amount. It withdraws amount tokens from the vault and transfer them to to, and burns the calculated amount of shares from from. So we can see that the minSharesOut is really wrong to use, because it actually burns the shares from from and not giving the shares to to. The correct implementation should use maxSharesIn, in order to specify the maximum amount of shares the user is willing to burn. It should look something like this:

/// @inheritdoc IERC4626RouterBase
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesIn
) public payable virtual override returns (uint256 sharesIn) {
    if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
        revert MinAmountError();
    }
}

As far as I know, this bug can't be exploited, because it only limits the users' functionallity, but it is a bug that needs to be fixed. In addition to limiting the users' functionallity, I think that users will prefer the maxSharesOut fix that I suggested, to limit the amount of shares that wil be burned and avoiding burning to much shares to get a small amount of tokens.

Confusing error messages

Another thing that is half related to the previous bug is the MinAmountError. It is correct in the redeem function, but in the other function I think that the errors should be more specific. I think that this is a more understood implementation (using the alternative withdraw function I suggested before):

/// @inheritdoc IERC4626RouterBase
function mint(
    IERC4626 vault, 
    address to,
    uint256 shares,
    uint256 maxAmountIn
) public payable virtual override returns (uint256 amountIn) {
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
        revert MaxAmountError();
    }
}
/// @inheritdoc IERC4626RouterBase
function deposit(
    IERC4626 vault, 
    address to,
    uint256 amount,
    uint256 minSharesOut
) public payable virtual override returns (uint256 sharesOut) {
    if ((sharesOut = vault.deposit(amount, to)) < minSharesOut) {
        revert MinSharesError();
    }
}
/// @inheritdoc IERC4626RouterBase
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesIn
) public payable virtual override returns (uint256 sharesIn) {
    if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
        revert MaxSharesError();
    }
}
/// @inheritdoc IERC4626RouterBase
function redeem(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 minAmountOut
) public payable virtual override returns (uint256 amountOut) {
    if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) {
        revert MinAmountError();
    }
}

The change is that the error that is reverting gives more information about the error happened. Reverting with a MinAmountError in every function can be very confusing.

[Joeysantoro]

https://github.com/fei-protocol/ERC4626/pull/10

[Joeysantoro]

part 1 is a duplicate

[GalloDaSballo]

Bumping the part 1 up to a Medium Finding, duplicate of #28

[GalloDaSballo]

Second finding is just one comment so I believe making this just the med report is fair