code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The _feePerClaim is a user controlled parameter which tops at tierLiquidity.prizeSize for a given _tier (see here for that). That means the CLAIMER can set arbitrary fees for a given call to claimPrize to increase maliciously the collected fees with

// PrizePool.sol#L455
claimerRewards[_feeRecipient] += _fee;

(see here)

and collect them with withdrawClaimRewards. The severity here is because winners will be losing part of their prizes (or everything if there is an issue with the _getTier function in TieredLiquidityDistributor.sol file that computes the wrong tierLiquidity.prizeSize) because of this "arbitrage" from the CLAIMER

(for the judges' confort, so they do not go from one file to another, here is the affected code)

// Vault.sol#L609
function claimPrizes(
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    uint96 _feePerClaim, <--------------------------------------------------
    address _feeRecipient                                                  |
  ) external returns (uint256) {                                           |
    if (msg.sender != _claimer) revert CallerNotClaimer(msg.sender, _claimer);
                                                                           |
    uint totalPrizes;                                                      |
                                                                           |
    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, // <--------------------------------------------- HERE
          _feeRecipient
        );
      }
    }

    return totalPrizes;
  }

// Vault.sol#L1046
function _claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    uint96 _fee,
    address _feeRecipient
  ) internal returns (uint256) {
    VaultHooks memory hooks = _hooks[_winner];
    address recipient;
    if (hooks.useBeforeClaimPrize) {
      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
    } else {
      recipient = _winner;
    }

    uint prizeTotal = _prizePool.claimPrize(
      _winner,
      _tier,
      _prizeIndex,
      recipient,
      _fee, <--------------------------------------------- HERE
      _feeRecipient
    );

    ...
  }

// PrizePool.sol#L407
function claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    address _prizeRecipient,
    uint96 _fee, <--------------------------------------------------
    address _feeRecipient                                          |
  ) external returns (uint256) {                                   |
    Tier memory tierLiquidity = _getTier(_tier, numberOfTiers);    |
                                                                   |
    if (_fee > tierLiquidity.prizeSize) { <---------------------- HERE
      revert FeeTooLarge(_fee, tierLiquidity.prizeSize);
    }

    ...

    if (_fee != 0) {
      emit IncreaseClaimRewards(_feeRecipient, _fee);
      claimerRewards[_feeRecipient] += _fee; <------------------------------- HERE
    }

    // `amount` is now the payout amount
    uint256 amount = tierLiquidity.prizeSize - _fee; <----------------------- if wrong tierLiquidity.prizeSize, then amount can be 0 

    emit ClaimedPrize(
      msg.sender,
      _winner,
      _prizeRecipient,
      lastClosedDrawId,
      _tier,
      _prizeIndex,
      uint152(amount),
      _fee,
      _feeRecipient
    );

    _transfer(_prizeRecipient, amount); <-------------------------- if the above situation is true, winner receives nothing

    return tierLiquidity.prizeSize;
  }

Tools Used

Manual analysis

Recommended Mitigation Steps

Hard-code the fees with a constant so that users will know exactly what fees are they gonna pay OR make it variable and add a function to set its value with a delay between calls to ensure the CLAIMER does not abuse his position doing the arbitrage previously discussed