minDepositAmount should apply to asset and not share

[code423n4]

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27

Vulnerability details

Impact

As asset and share are different things, the user may deposit the ASSET amount which is <= minDepositAmount IF ITS SHARE is > minDepositAmount.

Proof of Concept

See ERC4626-Cloned.sol

function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    require(shares > 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);
  }

In the deposit function above, the system checks if shares <= minDepositAmount(), if yes, revert the deposit

require(shares > minDepositAmount(), "VALUE_TOO_SMALL");

The system tries to compare shares against assets (Deposit amount). Assets and shares are related but they are completely different things. For example,

In a new empty vault, user A and user B make deposits

User A has 10 assets which are 10 shares
User B has 20 assets which are 20 shares

Here, the assets and shares are the same.

However, if the system withdraws 6 assets from the vault for some purpose

Total tokens = 24,
Total shares = 30,

In this case, the assets and shares are not the same

User A has 10 shares (which is 10 / 30 * 24 = 8 assets)
User B has 20 shares (which is 20 / 30 * 24 = 16 assets)

Let say,

If user C deposits 24 assets, he will get 30 shares [24 / (24 + 8 + 16) * (10 + 20 + 30)]

With the current code,

If minDepositAmount() = 25, his deposit will pass because 30 > 25. This is wrong because the deposit should fail as 24 < 25. We should verify by assets (NOT shares)

Tools Used

Manual

Recommended Mitigation Steps

Change the following code

function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    --- require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
    +++ 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);
  }

[c4-judge]

Picodes marked the issue as duplicate of #486

[c4-judge]

Picodes marked the issue as satisfactory