PoolTogether - ni8mare's results

Lines of code

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

Vulnerability details

Impact

Anyone can call mintYieldFee in the Vault contract to mint shares for themselves, which are supposed to go to the _yieldFeeRecipient.

Proof of Concept

The comment above the function mintYieldFee says the following:

@notice Mint Vault shares to the yield fee _recipient.

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

So, the intention is clearly to mint Vault shares to the _yieldFeeRecipient. But, the function lacks any check to verify whether the _recipient argument is equal to the _yieldFeeRecipient. Hence, it allows, anyone to mint themselves shares by subtracting that amount from the _yieldFeeTotalSupply.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended that the function include a modifier to prevent this or check that the _recipient be equal to the _yieldFeeRecipient

require(_yieldFeeRecipient == _recipient, "_recipient is not the same as _yieldFeeRecipient");

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Theoretically, it is possible to mint more than the maxMint amount using the mintYieldFee function in the Vault contract.

Proof of Concept

The functions in Vault contract like mint, mintWithPermit call the _beforeMint function which checks whether _shares are greater than maxMint amount and reverts if it's indeed the case. But, the mintYieldFee amount does not check this. The yieldFeeTotalSupply is a unit256 which gets subtracted with shares, but the maxMint only allows for uint96. So theoretically, it is possible to mint more shares than maxMint, because shares are a uint256 value, not uint96 in the function.

But, there is a scenario where the protocol should be mindful. There is no limit on the decimals of the asset tokens that are used as the vaults can be created permissionless by anyone. And shares also have the same amount of decimals. So, yieldFeeTotalSupply could accumulate in amount (even if the underlying value is less) to be greater than max(uint96) value, which makes it possible for a share greater than maxMint(max(uint96)) amount to be subtracted from yieldFeeTotalSupply and hence possible to mint shares greater than max(uint96) value.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended that the protocol add the maxMint check in the mintYieldFee function.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

It is theoretically possible for the deposit amount to mint shares more than the maxMint amount

Proof of Concept

The deposit function has a check for maxDeposit and reverts if the deposit value is more than max(uint96). But, it does not check the shares to be less than maxMint amount and hence bypasses this check. Theoretically, if the assets are equal to max(uint96) and if the vault is under-collateralised, the ratio of _assetUnit to _exchangeRate is greater than or equal to 2, then the calculation in _convertToShares:

_assets.mulDiv(_assetUnit, _exchangeRate, _rounding); could return a value more than the maxMint amount. This is possible in those scenarios where the _assetUnit is a big enough number (possible, as there is no limit on the decimals of the underlying asset, and _assetUnit = 10 ** super.decimals()) and the Vault is severely under-collateralized.

Tools Used

Manual Review

Recommended Mitigation Steps

Include the maxMint check in the deposit function to prevent this problem.

Assessed type

Invalid Validation