code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L965-L974

Vulnerability details

Impact

A user could accumulate more shares than the defined maximum limit.

Proof of Concept

Suppose a user (_receiver) currently holds some shares and the maxMint limit is defined. This user can still call the mint or mintWithPermit function multiple times with _shares values that, in sum, exceed the maxMint limit. This is because the mint function's implementation does not account for the total shares a user could have after multiple mint transactions, or what they had before calling this

Here is the mint()'s function code:

function mint(uint256 _shares, address _receiver) public virtual override returns (uint256) {
    uint256 _assets = _beforeMint(_shares, _receiver);

    _deposit(msg.sender, _receiver, _assets, _shares);

    return _assets;
}

Which calls the _beforeMint() function

  /**
   * @notice Compute the amount of assets to deposit before minting `_shares`.
   * @param _shares Amount of shares to mint
   * @param _receiver Address of the receiver of the vault shares
   * @return uint256 Amount of assets to deposit.
   */
  function _beforeMint(uint256 _shares, address _receiver) internal view returns (uint256) {
    if (_shares > maxMint(_receiver)) revert MintMoreThanMax(_receiver, _shares, maxMint(_receiver));
    return _convertToAssets(_shares, Math.Rounding.Up);
  }

Tools Used

Manual Audit

Recommended Mitigation Steps

Modify the _beforeMint function to include a check that accounts for the sum of _shares to be minted and the shares already minted by the _receiver. This will ensure that users cannot exceed the maxMint limit through multiple mint transactions.