PoolTogether - eyexploit's results

[NC-01] Update _claimer only for non-zero value

During Vault deployment, Address of Claimer contract can be set with _setClaimer function which called internally in the constructor.

It is commented that the passed param:

// @dev claimer_ can be set to address zero if none is available yet.

So if deployer decide it to change later for above reason, s/he still would able to call _setClaimer function passing zero address by default in the constructor. Hence, updating the storage slot of Vault contract unnecessary.

NOTE: Even though it re-initialized with zero address, it still cost gas of amount of updating storage slot.

Proof of Concept

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

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

Recommendation

Remove _setClaimer from constructor, as it can be changed later with setter function. or

Add this check in the Vault#constructor() to avoid initializing the storage slot with default value.

if (claimer_ != address(0)) 
	_setClaimer(claimer_); 

[NC-02] Typo in comments

1. File: ObservationLib.sol

/**
 * @dev Sets max ring buffer length in the Account.observations Observation list.
 *         As users transfer/mint/burn tickets new Observation checkpoints are recorded.
 *         The current `MAX_CARDINALITY` guarantees a one year minimum, of accurate historical lookups.
 * @dev The user Account.Account.cardinality parameter can NOT exceed the max cardinality variable.
 *      Preventing "corrupted" ring buffer lookup pointers and new observation checkpoints.
 */
uint16 constant MAX_CARDINALITY = 365; // 1 year

Typo in above natspec, it should be "The current MAX_CARDINALITY guarantees a one year maxium, of accurate historical lookups".

As after completing 365 checkpoints, its start overwriting previous observation, also explained in above natspec.

Recommendation change minimum -> maximum

2. File: UD34x4.sol

/// @notice Casts an UD34x4 number into UD60x18.
/// @dev Requirements:
/// - x must be less than or equal to `uMAX_UD2x18`. <= WRONG
function intoUD60x18(UD34x4 x) pure returns (UD60x18 result) {
  uint256 xUint = uint256(UD34x4.unwrap(x)) * uint256(1e14);
  result = UD60x18.wrap(xUint);
}

/// @notice Casts an UD34x4 number into UD60x18. <= WRONG
/// @dev Requirements:
/// - x must be less than or equal to `uMAX_UD2x18`.  <= WRONG
function fromUD60x18(UD60x18 x) pure returns (UD34x4 result) {
  uint256 xUint = UD60x18.unwrap(x) / 1e14;
  if (xUint > uMAX_UD34x4) {
    revert PRBMath_UD34x4_fromUD60x18_Convert_Overflow(x.unwrap());
  }
  result = UD34x4.wrap(uint128(xUint));
}

Its look like the comments for intoUD60x18() copied into fromUD60x18(), which is wrong for fromUD60x18. As fromUD60x18() casts UD60x18 type to UD34x4 type.

Also the range define for x in @dev comment is incorrect for both intoUD60x18() and fromUD60x18() function. Please follow the recommendation section below.

Recommendation Change current implementation to below;

/// @notice Casts an UD34x4 number into UD60x18.
/// @dev Requirements:
/// - x must be less than or equal to `uMAX_UD60x18`.
function intoUD60x18(UD34x4 x) pure returns (UD60x18 result) {
  uint256 xUint = uint256(UD34x4.unwrap(x)) * uint256(1e14);
  result = UD60x18.wrap(xUint);
}

/// @notice Casts an UD60x18 number into UD34x4 .
/// @dev Requirements:
/// - x must be less than or equal to `uMAX_UD34x4`.
function fromUD60x18(UD60x18 x) pure returns (UD34x4 result) {
  uint256 xUint = UD60x18.unwrap(x) / 1e14;
  if (xUint > uMAX_UD34x4) {
    revert PRBMath_UD34x4_fromUD60x18_Convert_Overflow(x.unwrap());
  }
  result = UD34x4.wrap(uint128(xUint));
}

[L-01] priceIndices length should match the winners array length

File: Claimer.sol

  function claimPrizes(
    Vault vault,
    uint8 tier,
    address[] calldata winners,
    uint32[][] calldata prizeIndices,
    address _feeRecipient
  ) external returns (uint256 totalFees) {
    uint256 claimCount;
    for (uint i = 0; i < winners.length; i++) {
      claimCount += prizeIndices[i].length;
    }

    uint96 feePerClaim = uint96(
      _computeFeePerClaim(
        _computeMaxFee(tier, prizePool.numberOfTiers()),
        claimCount,
        prizePool.claimCount() 
      )
    );

    vault.claimPrizes(tier, winners, prizeIndices, feePerClaim, _feeRecipient);
    return feePerClaim * claimCount;
  }

The claimPrize() function allows the caller to claim prizes on behalf of others, for which caller have to pass the mentioned parameters for vault. Its not ensure that the winners and prizeIndices are of equal length, which means any mismatch in length could result at unexpected outcome in the execution of claimPrize() function.

Recommendation

Add a below check to mitigate issue;

   if(winners.length != prizeIndices.length) 
	revert LengthNotMatches();