PoolTogether - Udsen's results

Lines of code

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

Vulnerability details

Impact

The Vault.mintYieldFee external function is used to mint Vault shares to the yield fee _recipient. The function is an external function and can be called by anyone since there is no access control. The function will revert only under following two conditions:

1. if the Vault is undercollateralized
2. if the _shares are greater than the accrued _yieldFeeTotalSupply.

The issue with this function is it allows the caller to set the _recipient (Address of the yield fee recipient). It does not use the _yieldFeeRecipient state variable which was set in the Vault.constructor as the yield fee recipient.

Which means any one can steal the available yield fee from this vault (As long as above two revert conditions are not satisfied) by minting shares to his own address or to any address of his choice.

Proof of Concept

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

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to use the _yieldFeeRecipient state variable value as the yield fee recipient inside the Vault.mintYieldFee external function and to remove the input parameter address _recipient from the Vault.mintYieldFee function. So that the caller will not be able to mint shares to any arbitory address of his choice and steal the yield fee of the protocol.

The updated function should be as follows:

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

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

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

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L951-L956 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L959 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026-L1027

Vulnerability details

Impact

The Vault._deposit function is used by the users to deposit _assets to the vault and mint vault shares to the recipient address. The amount of _assets are transferred to the Vault as follows:

  SafeERC20.safeTransferFrom(
    _asset,
    _caller,
    address(this),
    _assetsDeposit != 0 ? _assetsDeposit : _assets
  );

The Vault.deposit function uses this _assets amount to calculate the number of shares to be minted to the _recipient address.

The issue here is if the underlying _asset is a fee on transfer token then the actual received amount to the vault will be less than what is referred in the Vault.deposit function _assets input parameter. But the shares to mint is calculated using the entire _assets amount.

This issue could be further aggravated since the _asset is again deposited to the _yieldVault and when needing to be redeemed will be withdrawn from the _yieldVault as well. These operations will again charge a fee if the _asset is a fee on transfer token. Hence the actual left _asset amount for particular user will be less than the amount he initially transferred in.

Hence when the user redeems the minted shares back to the _assets, the contract will not have enough assets to transfer to the redeemer thus reverting the transaction.

Proof of Concept

      SafeERC20.safeTransferFrom(
        _asset,
        _caller,
        address(this),
        _assetsDeposit != 0 ? _assetsDeposit : _assets
      );

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

    _yieldVault.deposit(_assets, address(this));

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

    _yieldVault.withdraw(_assets, address(this), address(this));
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to compute the _assets amount balance of the contract before and after the safeTransferFrom call and get the difference between the two as the actually transferred amount to the Vault. Then this actually transferred amount can be converted to shares and mint the correct amount of shares to the recipient.

  uint256 balanceBefore = _asset.balanceOf(address(this));

      SafeERC20.safeTransferFrom(
        _asset,
        _caller,
        address(this),
        _assetsDeposit != 0 ? _assetsDeposit : _assets
      );

  uint256 balanceAfter = _asset.balanceOf(address(this));  

  uint256 transferredAmount = balanceAfter - balanceBefore;

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L618-L629 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1053 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1068

Vulnerability details

Impact

The Vault.claimPrizes function is used to claim prizes for the winners of the prizes. This is done by iterating through two nested for loops. One for loop iterates through the number of winners and other for loop iterates through the _prizeIndices.

Inside these nested loops the Vault._claimPrize() internal function is called. In the Vault._claimPrize() function, the hooks.useBeforeClaimPrize and hooks.implementation.afterClaimPrize hooks of a particular winner can be called. These hooks are set by the winner himself calling the Vault.setHooks function.

Hence a malicious winner can revert the transaction inside the hooks.useBeforeClaimPrize or hooks.implementation.afterClaimPrize hook function, thus making the entire prize claim for other winners to be Denied as well(DoS).

This will make it impossible for the _claimer to distribute the prizes among the winners.

Proof of Concept

    for (uint w = 0; w < _winners.length; w++) {
      uint prizeIndicesLength = _prizeIndices[w].length;
      for (uint p = 0; p < prizeIndicesLength; p++) {
        totalPrizes += _claimPrize(
          _winners[w],
          _tier,
          _prizeIndices[w][p],
          _feePerClaim,
          _feeRecipient
        );
      }
    }

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

      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);

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

      hooks.implementation.afterClaimPrize(

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to either omit the hook functionality from the winners or if hooks are an essential part of the protocol then it is recommended to allow the winner or a claimer to claim the prizes individually rather than iterating through an array of all winners.

Assessed type

DoS

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278-L281 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299-L306 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L348 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335-L342

Vulnerability details

Impact

In the PrizePool.constructor the drawManager state variable is set as follows:

drawManager = params.drawManager;

But it accepts even if the params.drawManager == address(0). There is no input validation for the address(0) as well.

The PrizePool.setDrawManager function is used to set the drawManager if not already set. But the problem here is that there is no access control for this function and any one can call this. A malicious user can front run the valid drawManager assignment transaction and set a malicious address as the drawManager. The other issue is once the drawManager is set it can not be updated due to the following conditional check. Hence the contract will have to redeployed.

if (drawManager != address(0)) { //@audit-issue - can be front run by a malicious user
  revert DrawManagerAlreadySet();
}

Hence the following two functions controlled by the onlyDrawManager modifier will be vulnerable to attacks, since the drawManager is a malicious address now.

1. PrizePool.withdrawReserve can withdraw tokens from the reserve to any address given in the _to parameter. Hence this function will be vulnerable.

2. PrizePool.closeDraw() function can close the current open draw. This function is only callable by the drawManager. Hence a malicious user can get control of this function thus making this function vulnerable.

Proof of Concept

    drawManager = params.drawManager;
    if (params.drawManager != address(0)) {
      emit DrawManagerSet(params.drawManager);
    }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278-L281

  function setDrawManager(address _drawManager) external {
    if (drawManager != address(0)) {
      revert DrawManagerAlreadySet();
    }
    drawManager = _drawManager;

    emit DrawManagerSet(_drawManager);
  }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299-L306

  function closeDraw(uint256 winningRandomNumber_) external onlyDrawManager returns (uint16) {

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L348

  function withdrawReserve(address _to, uint104 _amount) external onlyDrawManager {
    if (_amount > _reserve) {
      revert InsufficientReserve(_amount, _reserve);
    }
    _reserve -= _amount;
    _transfer(_to, _amount);
    emit WithdrawReserve(_to, _amount);
  }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335-L342

Tools Used

Manaul Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to check for address(0) in the constructor when drawManager is set or to implement access control in the PrizePool.setDrawManager function so that only the admin of the contract can call this function to set the drawManager if it is not set already.

Assessed type

Invalid Validation

1. ARITHMETIC OPERATIONS CAN BE UNCHECKED DUE TO PREVIOUS CONDITIONAL CHECKS

_reserve -= _amount; can be unchecked to save gas, since previous conditional check makes sure that _amount <= _reserve.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L336-L339 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L671 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L538 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L459 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L490

2. REDUNDANT MSTORE ASSIGNMENT IN THE PrizePool._computeNextNumberOfTiers() function.

In the PrizePool._computeNextNumberOfTiers() the following code snippet is used to update the _nextNumberOfTiers value as shown below:

_nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
  ? _nextNumberOfTiers
  : MINIMUM_NUMBER_OF_TIERS;

There is a redundant assignment of the _nextNumberOfTiers variable to itself when _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS.

This can be optimized as follows:

if(!(_nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS)){
    _nextNumberOfTiers = MINIMUM_NUMBER_OF_TIERS;
}

This will save an extra MLOAD.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L785-L787

3. USE LOCAL MEMORY VARIABLE INSTEAD OF STATE VARIABLE

The PrizePool._computeVaultTierDetails function is used to compute the data to determine the winner of a prize. In the function there is an internal call made to teh _tierOdds function as shown below:

tierOdds = _tierOdds(_tier, numberOfTiers); 

The numberOfTiers used in this function call is a state variable. Inplace of this state variable the _numberOfTiers memory variable can be used instead. This will save an extra SLOAD.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L901

4. ARITHMETIC OPERATION CAN BE SIMPLIFIED TO SAVE GAS

The Vault._currentExchangeRate function, is used to calculate the exchange rate between the amount of assets withdrawable from the YieldVault and the amount of shares minted by this Vault. In the function there is a calculation as follows to compute the _withdrawableAssets when _withdrawableAssets > _totalSupplyToAssets, as shown below:

  _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);

Here the yield is removed from the _withdrawableAssets amount.

The above calculation can be simplified as follows to save gas: and the above calculation can be commented for ease of understanding later.

  _withdrawableAssets = _totalSupplyToAssets;

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

5. REDUNDANT CONDITIONAL CHECK CAN BE OMITTED TO SAVE GAS

In the Vault.liquidate function the following check is performed to verify that _tokenOut is the vault share.

if (_tokenOut != address(this))
  revert LiquidationTokenOutNotVaultShare(_tokenOut, address(this));

But the same check check is performed inside the _liquidatableBalanceOf(_tokenOut) function call as well. Hence the above mentioned check inside the Vault.liquidate is a redundant check that can be omitted to save gas.

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

6. ARRAY LENGTH MISMATCH IN THE Vault.claimPrizes COULD LEAD TO WASTE OF GAS

Vault.claimPrizes is used to calim the prizes for the winners. There are two arrays used in the function as shown below:

address[] calldata _winners,
uint32[][] calldata _prizeIndices,

These two arrays are used iterate through inside a nested for loop. But before the for loop the length of the above two arrays are not checked for equality. Hence if there is an array length mismath the transaction will revert after entering the for loop thus leading to a considerable gas loss to the _claimer.

Hence it is recommended to check _winners.length == _prizeIndices.length at the beginning of the function before entering the for loop.

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