code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the Claimer.sol Contract, The function claimPrizes is a function used to claim prizes for the winners. This function takes in some user inputs which includes some arrays. However the function does check the arrays before passing in the values.

Proof of Concept

  function claimPrizes(
    Vault vault,
    uint8 tier,
    address[] calldata winners,
    uint32[][] calldata prizeIndices,
    address _feeRecipient
  ) external returns (uint256 totalFees) {
    uint256 claimCount;
    //@audit-issue   verify array.length before iterating
    for (uint i = 0; i < winners.length; i++) {
      claimCount += prizeIndices[i].length;
    }

As seen above, the function takes in two arrays and iterates over them using the length of one. This can cause out-of-bound errors which are unhandled in the contract if winners.length > priceIndices[i].length. However if winners.length < priceIndices[i].length, it does not revert immediately instead it calls the vault contract with the defective parameters where they are iterated upon

    vault.claimPrizes(tier, winners, prizeIndices, feePerClaim, _feeRecipient);

The variable lengths are unhandled in the Vault.sol contract as well In the function claimPrizes, we have:


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

Tools Used

Manual Review

Recommended Mitigation Steps

Add a require statement require(winners.length == prizeIndices[].length); to check that the lengths are equal

Assessed type

Loop

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)