PoolTogether - Praise's results

Lines of code

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

Vulnerability details

Impact

Anyone can mint vault shares to themselves, whereas it is supposed to be minted solely to the Yield Fee recipient(yieldFeeRecipient_)

Proof of Concept

taking a look at the constructor, i see yieldFeeRecipient_. as one of the constructor's input

  constructor(
    IERC20 asset_,
    string memory name_,
    string memory symbol_,
    TwabController twabController_,
    IERC4626 yieldVault_,
    PrizePool prizePool_,
    address claimer_,
    address yieldFeeRecipient_,//here
    uint256 yieldFeePercentage_,
    address owner_
  ) ERC4626(asset_) ERC20(name_, symbol_) ERC20Permit(name_) Ownable(owner_) {

This means the Vault.sol has a default yield fee recipient set by the protocol themselves.

AND also taking a look at the comment above the mintYieldFee function, i noticed the _recipient is supposed to be the yield Fee Recipient set by the protocol in the constructor when deploying vault.sol.

 /**
   * @notice Mint Vault shares to the yield fee `_recipient`.
   * @dev Will revert if the Vault is undercollateralized
   *      or if the `_shares` are greater than the accrued `_yieldFeeTotalSupply`.
   * @param _shares Amount of shares to mint
   * @param _recipient Address of the yield fee recipient
   */

But the issue here is that there's no check to ensure that the address passed into mintYieldFee function as _recipient is the same address set by the protocol as 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);
  }

Therefore anyone can mint vault shares to themselves.

Tools Used

Manual Review

Recommended Mitigation Steps

put a check to ensure that the address passed into mintYieldFee function as _recipient is the same address set by the protocol as yieldFeeRecipient_

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Attacker can drain tokens from the reserve.

Proof of Concept

This is due to setDrawManager() function having no access control implemented

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

    emit DrawManagerSet(_drawManager);
  }

An attacker can override the address set in the constructor as drawManager to his own address via the setDrawManager() function that has no access control and bypass the access control on withdrawReserve() function. Thereby draining the funds from the reserve.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add access control to setDrawManager() function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The setDrawManager() can be used to override the drawManager set in the constructor, because it has no access control implemented on it. A malicious actor/ attacker could use this to takeover this role(drawManager) and have access to functions that are only callable by the role (drawManager)

Proof of Concept

This is possible since the setDrawManager() has no access control implemented on it

Tools Used

Manual Review

Recommended Mitigation Steps

Add access control on the setDrawManager() function.

Assessed type

Access Control