code423n4 commented 1 year ago

Lines of code

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

VaultHooks are a way for for winner to call an external smart contract when they win a prize. Any PoolTogether user can set a hook with setHooks. _claimPrize calls beforeClaimPrize and afterClaimPrize hooks of the VaultHooks for every winner. Because the individual winners have control over the address chosen for their own hooks, they can choose a malicious crafted contract that consumes large amounts of gas or reverts. A hook that disrupts the call will cause the user calling _claimPrize to manually remove the offending winner, and if there are several winners with a problematic hook, the caller of _claimPrize may spend extensive gas and time to call _claimPrize, which can further reduce the incentive to assist the protocol with prize claims in the future.

The winner can selectively set the hook to cause the caller of _claimPrize to waste time and gas, then later the winner can set the hook to a different contract and call _claimPrize for their own address, meaning they will only disrupt protocol operations without losing out on their rewards.

Proof of Concept

Any user can call claimPrizes to claim prizes on behalf of others. An array of winners is a function argument https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L80

During the claiming process, the winner's beforeClaimPrize and beforeClaimPrize hooks will be called at the external contract specified by the winner 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

The external contract can implement these hooks to prevent the for loop from completing, disrupting the caller's task of distributing rewards. These are example hooks that revert from an our of gas error or a revert.

  function beforeClaimPrize(
    address winner,
    uint8 tier,
    uint32 prizeIndex
  ) external override returns (address) {
    uint a;
    while(true) {
      a += 1;
    }
  }

  function afterClaimPrize(
    address winner,
    uint8 tier,
    uint32 prizeIndex,
    uint256 payout,
    address recipient
  ) external override {
    revert dos();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Documentation should clarify a process to be taken in cases where the hook set by the winner disrupts claiming operations. Consider adding a protocol incentive to prevent users from setting disruptive hooks, for example a 10% reduction from the original prize amount for every 24 hours that the winner has a disruptive hook set.Claimers could use a simulation process on a fork to confirm that the _claimPrize process will complete without error, but a user who frontruns the process means this is not foolproof, especially if prizes are released at predictable times.

Assessed type

DoS

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #465