PoolTogether - shaka's results

Lines of code

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

Vulnerability details

In Vault.sol the _feeRecipient is supposed to be the address that receives the fee amount when yield is captured (tracked in _yieldFeeTotalSupply), as we can see in different comments of the contract, including this one:

  /**
   * @notice Increase yield fee balance accrued by `_yieldFeeRecipient`.
   * @param _shares Amount of shares to increase yield fee balance by
   */
  function _increaseYieldFeeBalance(uint256 _shares) internal {
    _yieldFeeTotalSupply += _shares;
  }

However, the mintYieldFee function allows anyone to mint yield fee shares to any address, not just the _yieldFeeRecipient:

  function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
  }

This implementation looks to be an oversight from the developers, as we do not see any use of _yieldFeeRecipient in the contract.

Impact

The yield fee shares can be minted to any address, not just the _yieldFeeRecipient.

Proof of Concept

Edit Liquidate.t.sol::testLiquidateAndMintFees to the following:

    vm.expectEmit();
-   emit MintYieldFee(address(this), bob, _yieldFeeShares);
+   emit MintYieldFee(address(this), address(111), _yieldFeeShares);

-   vault.mintYieldFee(_yieldFeeShares, bob);
+   vault.mintYieldFee(_yieldFeeShares, address(111));

-   assertEq(vault.balanceOf(bob), _yieldFeeShares);
+   assertEq(vault.balanceOf(address(111)), _yieldFeeShares);

The test will still pass.

Tools Used

Manual inspection.

Recommended Mitigation Steps

- function mintYieldFee(uint256 _shares, address _recipient) external {
+ function mintYieldFee(uint256 _shares) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
-   _mint(_recipient, _shares);
+   _mint(_yieldFeeRecipient, _shares);

-   emit MintYieldFee(msg.sender, _recipient, _shares);
+   emit MintYieldFee(msg.sender, _yieldFeeRecipient, _shares);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

A malicious user can make Vault::claimPrizes execution fail by reverting on IVaultHooks::beforeClaimPrize and IVaultHooks::afterClaimPrize calls. These hooks allow to perform arbitrary operations before and after the prize is claimed.

Another way to make the process fail is returning an invalid recipient address from IVaultHooks::beforeClaimPrize, such the zero address or a blacklisted address for the prize token.

Even if the claimer bot simulates the prize claim transaction to avoid revert, the malicious user can still front-run the transaction and modify the code in the hooks to make it fail.

Impact

The call to Vault::claimPrizes will fail and it will have to be retried after removing the malicious winner address from the list of winners. This will suppose an extra gas cost for the Claimer contract. Note that there is the chance that there are more than one malicious winner address, so the process will have to be repeated for each of them, at the point that it might become impractical claiming prizes in batches.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Wrap hooks.implementation.beforeClaimPrize, _prizePool.claimPrize and hooks.implementation.afterClaimPrize in a try/catch block and return 0 in case of an exception.

Assessed type

Error

Lines of code

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

Vulnerability details

When the LiquidationPair contract calls Vault::liquidate, _amountIn of prize token is deposited into PricePool and _amountOut of vault token is minted to the liquidator.

_mint internal function downcasts uint256 to uint96 and this can result in an underflow silently, so the liquidator can receive less tokens than expected.

The issue will happen when _amountOut, and thus, the liquidatable balance, are greater than 2^96 - 1 (the maximum value of uint96). This value is equivalent to 79_228_162_514 units of a token with 18 decimals and 79_228 units of a token with 24 decimals, so it is not unrealistic to expect this to happen.

Impact

The liquidator can receive less tokens than expected when liquidating a vault.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Use OpenZeppelin's SafeCast library to convert uint256 to uint96.

Assessed type

Under/Overflow

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1155 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L48

Vulnerability details

Vault::_transfer overrides the ERC20::_transfer function so that the transfer is performed using the TwabController contract. In the implementation _shares is downcasted to uint96 and this can result in an underflow silently.

There are nat spec comments indicating that the balance in TwapController is stored in uint96, which would mean that there is no real risk of underflow. However, the TwapController contract uses uint112 to store the balance, so the risk of underflow is real.

File: TwabLib.sol

47  struct AccountDetails {
48    uint112 balance; // <== not uint96
49    uint112 delegateBalance;
50    uint16 nextObservationIndex;
51    uint16 cardinality;
52  }

Impact

Integrations of other contracts or protocols with the Vault contract may result in accounting errors due to the amount of shares transferred being less than expected due to silent underflow.

Proof of Concept

Foundry test:

  mapping(address => uint256) userToBalance;

  function testTransferUnderflow() external {
    vm.startPrank(alice);

    uint256 _amount = type(uint112).max;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(type(uint96).max, alice);
    vault.deposit(type(uint96).max, alice);
    vm.stopPrank();
    assertEq(vault.balanceOf(alice), uint256(type(uint96).max) * 2);

    // Alice has now a balance of type(uint96).max * 2
    // Let's imagine some integration of another contract with the Vault contract
    // that performs the following operations:

    // 1. Alice approves all the Vault shares to the contract
    vm.prank(alice);
    vault.approve(address(this), type(uint256).max);

    // 2. The contract transfers all the Vault shares from Alice and registers them
    // in in a mapping
    uint256 aliceBalance = vault.balanceOf(alice);
    vault.transferFrom(alice, address(this), aliceBalance);
    userToBalance[alice] = aliceBalance;

    // 3. Only type(uint96).max shares are transfered, but the double of that amount
    // is registered as transfer underflows silently. So alice can now transfer the
    // remaining balance from the vault.
    vm.prank(alice);
    vault.transfer(bob, type(uint96).max);
  }

Tools Used

Manual inspection.

Recommended Mitigation Steps

Use OpenZeppelin's SafeCast library to convert uint256 to uint96.

Assessed type

Under/Overflow