PoolTogether - zzzitron's results

Lines of code

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

Vulnerability details

Impact

When the Vault.withdraw() function is called, a maximum of type(uint96).max shares are being burnt subsequently: Vault.withdraw()-> Vault._withdraw()-> Vault._burn burns uint96(_shares), see Vault.sol line 1139.

A malicious user can exploit this in the following way:

1. A malicious user deposits for example two times a value of type(uint96).max underlying assets into the Vault, calling two times the function Vault.deposit(). They can't deposit more in a single transaction because type(uint96).max is the maximum value to deposit.

2. Then the malicious user calls Vault.withdraw() with a higher value of assets to withdraw than type(uint96).max, for example they withdraw (2 * type(uint96).max) which is the total amount of assets they depositted before.

3. Now what happens is that the Vault.sol contract only burns type(uint96).max shares for the user, but transfers 2 * type(uint96).max underlying assets to the malicious user, which is the total amount they depositted before.

4. This happens because Vault._burn() only burns uint96(shares) shares of the malicious users - see Vault.sol line 1155.

5. Now the malicious user has still vault shares left but they withdrew the total amount of their depositted assets.

6. Now the vault transferred the total amount of the malicious user's assets back to them, and the malicious user has still shares left to withdraw even more assets that are now being stolen from assets depositted by other users.

7. Or if the malicious user was the first depositor, they wait until another user deposits and the malicious user can now withdraw the other users depositted assets since the malicious user still has Vault shares left.

8. Or if the malicious user is not the first depositor, they use a flashLoan or flashMint to deposit multiple times type(uint96).max assets into the vault, then withdraw their deposit, pay back the flashLoan or flashMint and they will still have enough vault shares left to steal all other users assets by withdrawing them.

In this way, other user's depositted assets can be stolen, as explained above.

Proof of Concept

Here is a POC, where the problem is illustrated:

https://gist.github.com/zzzitron/397790302ca95aa3fbf05694ae1497ab

Tools Used

Manual review

Recommended Mitigation Steps

Consider adjusting the Vault._burn function to not convert from uint256 to uint96 when burning shares.

Assessed type

Math

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L573 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394-L399 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L704-L706 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1229-L1231

Vulnerability details

Impact

The function Vault.mintYieldFee() is missing access control and can be called by anybody. It also allows to specify any _recipient to receive the shares for the yield fee. It ignores the state variable Vault._yieldFeeRecipient, which is the address of the yield fee recipient, that can be set via the constructor of the Vault.sol contract or via Vault.setYieldFeeRecipient() by the owner of the vault.

All these issues potentially lead to undesired behavior:

1. When Vault.liquidate() is called, the function Vault._increaseYieldFeeBalance gets called which increases the _yieldFeeTotalSupply. After Vault.liquidate() was called, a malicious actor can call Vault.mintYieldFee() before the liquidator is calling the function, in order to frontrun and steal the shares of the yield fee (_yieldFeeTotalSupply), by specifying the malicious actor's address for the _recipient function parameter and minting the shares for themselves. Vault._yieldFeeRecipient gets ignored.

2. Also the liquidator could send the shares for the yield fee anywhere when calling Vault.mintYieldFee(), by specifying any address for the function parameter _recipient. Vault._yieldFeeRecipient gets ignored.

3. The owner of the Vault.sol contract calls Vault.setYieldFeeRecipient() in order to specify who should receive the shares for the yield fee. But when Vault.mintYieldFee() is called afterwards, the value Vault._yieldFeeRecipient that the owner set is ignored, and the shares for the yield fee are being sent to some other recipient potentially.

Because of these issues, yield fee shares could be lost: either because they could be stolen by a malicious actor or they could be send to an inadvertent recipient by a liquidator. The Vault owner can't influence who will receive the shares for the yield fees.

Proof of Concept

Here is a POC:

https://gist.github.com/zzzitron/7ec08fc8428ebe86e193dc35717a997f

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the function parameter _recipient from Vault.mintYieldFee() and send the shares to the specified Vault._yieldFeeRecipient of the Vault.sol contract.

Or consider introducing access control to Vault.mintYieldFee():

// Vault.sol
394  function mintYieldFee(uint256 _shares, address _recipient) external {
395    if (msg.sender != address(_liquidationPair)) {
396        // revert

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L307-L315 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L818-L819 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394-L399 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L868 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L886

Vulnerability details

Impact

Vault.availableYieldBalance() correctly checks, whether the Vault is undercollateralized, and returns 0 if that is the case, since there must not be more shares minted than underlying assets available. It does so by calculating the needed amount of assets for the Vault to be collateralized, based on _totalShares(). _totalShares() is returning the sum of _totalSupply() plus Vault._yieldFeeTotalSupply. This is important since Vault._yieldFeeTotalSupply can also be converted to shares via Vault.mintYieldFee().

The problem is that the Vault contract also uses another function to determine the vault collateralization: Vault._requireVaultCollateralized(), where subsequently Vault._currentExchangeRate() is called to do the calculations for the collateralization, but there is a flaw:

• Vault._currentExchangeRate() is using Vault._totalSupply() to determine the assets needed to be collateralized (Vault.sol line 1169)
• Vault._totalSupply() doesn't take into account Vault._yieldFeeTotalSupply
• Now when the needed assets are calculated, the calculation uses the result of Vault._totalSupply(), which leads to the problem that fewer assets are calculated (_totalSupplyToAssets) than needed for the Vault to be collateralized, because Vault._yieldFeeTotalSupply wasn't taken into account (Vault.sol line 1170)
• Finally the exchange rate is calculated wrongly as a result of the wrongly calculated _totalSupplyToAssets on line 1183, returning a wrong exchange rate which is 1e18, but should indeed be smaller than 1e18 to indicate that the vault is undercollateralized.
• Then the return value of Vault._currentExchangeRate() which was wrongly returned as 1e18 is used to determine whether the vault is collateralized.
• After Vault._requireVaultCollateralized() is checked and doesn't revert, more shares can be minted despite the fact that potentially any additional share minted is not covered by the available assets, so minting means to render the Vault undercollateralized as a result.

As a consequence, more shares can be minted than there are underlying assets available, which should not be allowed and which should be reverted - see comment in Vault.sol line 308.

Also as another issue, Vault._currentExchangeRate() can potentially and wrongly return 1e18 (or 10**underlyingAssetDecimals if the underlying asset doesn't use 18 decimals precision) as exchange rate despite that the exchange rate should be smaller than 1e18 because Vault._yieldFeeTotalSupply was not taken into account as shown above. That means that there can be potential issues wherever Vault._currentExchangeRate() is used in the context of the described issue above. Just as an example when Vault._convertToAssets() gets called it uses Vault._currentExchangeRate() (Vault.sol line 886) for its calculations. The same applies to Vault._convertToShares() in Vault.sol line 868.

Since there are multiple issues, I consider this report's severity as high.

Proof of Concept

Here is a POC, where at the end of the POC it can be observed that vault.isVaultCollateralized() still returns true, despite that vault.availableYieldBalance() is returning 0 indicating that minting should not be possible any more since there are not enough underlying assets available. Then another single share is minted successfully which leads to the Vault being undercollateralized.

https://gist.github.com/zzzitron/2a5f748a376f121d521211b97c4f781a

Tools Used

Manual review

Recommended Mitigation Steps

Consider adjusting Vault._currentExchangeRate() to use _totalShares() to determine the total shares for which underlying assets should be available:

// Vault.sol
1168  function _currentExchangeRate() internal view returns (uint256) {
1169    uint256 _totalSupplyAmount = _totalShares(); // @audit: consider using _totalShares()

Assessed type

Math