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

12 stars 7 forks source link

Missing Condition Validator in claimPrizes function #159

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/main/src/Vault.sol#L607-L632

Vulnerability details

Impact

A condition that is necessary to ensure function claimPrizes(...){...} at Vault.sol don't give possible false misleading results is important at Line 617, right before the implementation of the for loops. Two major dynamic array variables make the for loops from L618-L620 possible (_winners & _prizeIndices). For these loops to work accurately without false results or errors _winner.length must be equal to _prizeIndices.length i.e _winner.length == _prizeIndices.length, if this condition is absent and not checked before the loops and either of the two array length is higher than the other:

  1. if _winner.length > _prizeIndices.length, then _prizeIndices[w].length; of line 619 will cause the reversion of code operation, since we would be calling _prizeIndices[w] that don't exist at some point in the loop of _winner array or we could simply have winners without prices.
  2. if _winner.length < _prizeIndices.length, then the code would run without a reversion but a smart contract inconsistency of prizes without claimers would be the case.

Proof of Concept

A condition validator to prevent this possibility is missing from this code @ https://github.com/GenerationSoftware/pt-v5-vault/blob/main/src/Vault.sol#L617-L632 and also at Line60-83 of Claimer.sol where the codes are connected @ https://github.com/GenerationSoftware/pt-v5-claimer/blob/main/src/Claimer.sol#L60-L83

Tools Used

Solidity, hardhat, chai, remix

Recommended Mitigation Steps

A simple error function to handle unexpected inequality scenario of _winner.length and _prizeIndices.length at line617 of Vault.sol is important.

Assessed type

Error

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

PierrickGT commented 1 year ago

The claimPrizes function has been removed.