code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Inconsistent behavior for canary claims in claimer #61

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Inconsistent behavior for canary claims in claimer

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Whenever claimer claims prizes he computes fees per claim without considering canary claims

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

src/Claimer.sol#L76

Tools Used

Recommended Mitigation Steps

Consider canary as claim

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

Assessed type

Error

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

Picodes commented 1 year ago

Keeping Med severity as fees amounts are affected