PoolTogether - josephdara's results

Lines of code

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

Vulnerability details

Impact

The _yieldFeeTotalSupply is the total accrued supply from the yield fee. In the layout of the contract, we have the yield fee receiver set in the constructor. https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L276 However, in the function to mint YieldFee as shares is free for anyone to call with no access control

Proof of Concept

//@audit-issue no access control, also can be claimed to an arbitrary address
  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 function has no access control, and can also be called anyone to mint the shares to the _recipient address provided by the caller. This causes direct theft of all protocol fees

Tools Used

Manual Review

Recommended Mitigation Steps

Add the onlyOwner modifier to the function. And also direct all funds to the yield fee receiver that has been preset

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L169-L177

Vulnerability details

Impact

_computeMaxFee is used to compute the fee for a particular tier that is to be claimed through function claimPrizes However this function returns the wrong fee whenever it is not _canaryTier being checked.

  function _computeMaxFee(uint8 _tier, uint8 _numTiers) internal view returns (uint256) {
    uint8 _canaryTier = _numTiers - 1;
    
    if (_tier != _canaryTier) {
      // canary tier
      return _computeMaxFee(prizePool.getTierPrizeSize(_canaryTier - 1));
    } else {
      return _computeMaxFee(prizePool.getTierPrizeSize(_canaryTier));
    }
  }

This returns the TierPrizeSize of a wrong tier no matter the tier is if it not the canary tier

if (_tier != _canaryTier) {
      return _computeMaxFee(prizePool.getTierPrizeSize(_canaryTier - 1));
    } 

It returns the TierPrizeSize of the _canaryTier - 1 instead of the tier that was passed in.

Proof of Concept

In the prizepool contract, we have the function

  function getTierPrizeSize(uint8 _tier) external view returns (uint96) {
    return _getTier(_tier, numberOfTiers).prizeSize;
  }

This returns the prizeSize of a specific tier as they do not all have the same prizeSize as seen in the getTier function

  function _getTier(uint8 _tier, uint8 _numberOfTiers) internal view returns (Tier memory) {
    Tier memory tier = _tiers[_tier];
    uint16 _lastClosedDrawId = lastClosedDrawId;
    if (tier.drawId != _lastClosedDrawId) {
      tier.drawId = _lastClosedDrawId;
      tier.prizeSize = uint96(
        _computePrizeSize(
          _tier,
          _numberOfTiers,
          fromUD34x4toUD60x18(tier.prizeTokenPerShare),
          fromUD34x4toUD60x18(prizeTokenPerShare)
        )
      );
    }
    return tier;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Pass in the appropriate tier if it is not the canary tier.

Assessed type

Other