`Vault.mintWithPermit()` can be DOSed

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L466-L469 https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L971-L974 https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L882-L887

Vulnerability details

Impact

Vault.mintWithPermit() uses a signature to approve the underlying asset. But the asset amount can be changed easily, so this method can be reverted and might be DoSed.

Proof of Concept

Vault.mintWithPermit() gets the share amount as an input and calculates the asset amount from the share. And then approves the asset amount with permit method.

    uint256 _assets = _beforeMint(_shares, _receiver);

    _permit(IERC20Permit(asset()), msg.sender, address(this), _assets, _deadline, _v, _r, _s);
    _deposit(msg.sender, _receiver, _assets, _shares);

The signature is generated using the exact value of the expected asset amount calculated from the share amount, and the resulting asset amount depends on the exchange rate of current vault.

  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);
  }

  function _convertToAssets(
    uint256 _shares,
    Math.Rounding _rounding
  ) internal view virtual override returns (uint256) {
    return _convertToAssets(_shares, _currentExchangeRate(), _rounding);
  }

So the resulting asset amount can be different from the value of transaction start time. And even an adversary can front-run and manipulate the exchange rate. If the resulting asset amount is different from the original one, the signature will not work as expected and mintWithPermit() will revert in most cases.

Tools Used

Manual Review

Recommended Mitigation Steps

We can input upper bound of the asset amount instead of the exact value of the asset amount.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as satisfactory

[PierrickGT]

Since most users will use the depositWithPermit function, I've removed the mintWithPermit one in this PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/15