Function mint() in ERC4626RouterBase would always revert because it uses shares instead of assets for allowance amount

[code423n4]

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L21

Vulnerability details

Impact

Function ERC4626RouterBase.mint() calls vault.mint() and vault's mint function calculates asset's amount and transfers underlying asset from caller but function ERC4626RouterBase.mint() uses shares amount when setting the approval for vault. This would cause ERC4626RouterBase.mint() to when assets amount is bigger than shares amount and this is common situation (assets per share increase because of the interest rate over time). users can't use AstariaRouter's mint() function which supports slippage and call to the mint() would revert most of the times.

Proof of Concept

This is mint() code in the ERC4626RouterBase contract:

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
    ERC20(vault.asset()).safeApprove(address(vault), shares);
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
  }

As you can see in the line ERC20(vault.asset()).safeApprove(address(vault), shares); code uses share amount to set spending allowance for vault address in underlying asset. shares shows vault token amount and code should have used maxAmountIn for setting allowance.
but in fact vault's mint() function transfers asset's amount after calculating it based on specified share amount. This is mint() function in ERC4626Cloned contract:

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

As you can see code transfers assets from caller after calculating it based on shares and the assets amount can be bigger than shares.
because most of the time shares amount is lower than assets amount (assets per share increase over time because of the interest rate) so this issue would cause calls to mint() function to revert. any other protocol integrating with Astaria using this function would have broken logic and also users would lose gas if they use this function. contract AstariaRouter inherits ERC4626RouterBase and uses mint() function so users can't call AstariaRouter.mint() which supports slippage allowance and they have to call vault's mint() directly and they may lose funds because of the slippage.

Tools Used

VIM

Recommended Mitigation Steps

use maxAmountIn for allowance because it's the max amount user gives allowance and vault should always use less than that amount and if vault tries to use more amount then call would revert as it should.

[c4-judge]

Picodes marked the issue as duplicate of #118

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #488