code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L618-L629 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1053 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1068

Vulnerability details

Impact

The Vault.claimPrizes function is used to claim prizes for the winners of the prizes. This is done by iterating through two nested for loops. One for loop iterates through the number of winners and other for loop iterates through the _prizeIndices.

Inside these nested loops the Vault._claimPrize() internal function is called. In the Vault._claimPrize() function, the hooks.useBeforeClaimPrize and hooks.implementation.afterClaimPrize hooks of a particular winner can be called. These hooks are set by the winner himself calling the Vault.setHooks function.

Hence a malicious winner can revert the transaction inside the hooks.useBeforeClaimPrize or hooks.implementation.afterClaimPrize hook function, thus making the entire prize claim for other winners to be Denied as well(DoS).

This will make it impossible for the _claimer to distribute the prizes among the winners.

Proof of Concept

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

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

      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);

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

      hooks.implementation.afterClaimPrize(

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to either omit the hook functionality from the winners or if hooks are an essential part of the protocol then it is recommended to allow the winner or a claimer to claim the prizes individually rather than iterating through an array of all winners.

Assessed type

DoS

Picodes commented 1 year ago

What prevents the claimer from just skipping the malicious hooks using the _winners array?

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #465

c4-judge commented 11 months ago

Picodes marked the issue as satisfactory