Vault does not conform to ERC4626

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L375-L377
https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L383-L385

Vulnerability details

Impact

Vault does not conform to ERC4626 which may break external integrations

Proof of Concept

The ERC4626 specification states that maxDeposit MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted.

Similarly, maxMint MUST return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted.

The PoolTogether V5 Vault connects to an external ERC4626-compliant Vault (_yieldVault) and deposits incoming assets in it. This means that maxDeposit and maxMint of the PoolTogether Vault must be constrained by the maxDeposit and maxMint of the external Vault.

Tools Used

Manual review, ERC-4626: Tokenized Vaults

Recommended Mitigation Steps

Replace the implementation of maxDeposit

  function maxDeposit(address) public view virtual override returns (uint256) {
    return _isVaultCollateralized() ? type(uint96).max : 0;
  }

with:

  function maxDeposit(address receiver) public view virtual override returns (uint256) {
    if (!_isVaultCollateralized()) return 0;

    uint256 yvMaxDeposit = _yieldVault.maxDeposit(receiver);
    return yvMaxDeposit < type(uint96).max ? yvMaxDeposit : type(uint96).max;
  }

Analogously, change the implementation of maxMint from

  function maxMint(address) public view virtual override returns (uint256) {
    return _isVaultCollateralized() ? type(uint96).max : 0;
  }

to:

  function maxMint(address receiver) public view virtual override returns (uint256) {
    if(!_isVaultCollateralized()) return 0;

    uint256 yvMaxMint = _yieldVault.maxDeposit(receiver);
    return yvMaxMint < type(uint96).max ? yvMaxMint : type(uint96).max;
  }

Assessed type

ERC4626

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[PierrickGT]

Fixed in the following PR: GenerationSoftware/pt-v5-vault#11

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as satisfactory

[asselstine]

Fixed in GenerationSoftware/pt-v5-vault#11