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

12 stars 7 forks source link

Order of Operations Allows Reentrancy Attack in `increaseReserve` Function #255

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L495-L502

Vulnerability details

The increaseReserve function allows anyone to deposit tokens into the Prize Pool reserve. However, a vulnerability in the function's order of operations makes it susceptible to a reentrancy attack. By exploiting this vulnerability, an attacker can repeatedly drain the Prize Pool's token balance. The vulnerable code snippet is as follows:

_reserve += _amount;
prizeToken.safeTransferFrom(msg.sender, address(this), _amount);

The vulnerability lies in the fact that the _reserve variable is updated before the token transfer takes place. This order of operations can be exploited in a reentrancy attack.

Impact

The impact of this vulnerability is that an attacker can repeatedly drain the Prize Pool's token balance, potentially leading to significant financial losses for participants or disrupting the intended functionality of the Prize Pool.

Proof of Concept

  1. The attacker deploys a malicious contract with a fallback function that recursively calls the increaseReservefunction.
  2. The attacker calls the increaseReserve function on the Prize Pool contract from their malicious contract.
  3. The increaseReserve function is invoked and increments the _reserve variable.
  4. Before the token transfer is completed, the fallback function of the attacker's contract is triggered and recursively calls the increaseReserve function.
  5. The _reserve variable is incremented again, and the process continues in a loop, allowing the attacker to drain the Prize Pool's token balance.

    Tools Used

    manual

    Recommended Mitigation Steps

    "checks-effects-interactions" pattern.

    
    function increaseReserve(uint104 _amount) external {
    prizeToken.safeTransferFrom(msg.sender, address(this), _amount);
    _reserve += _amount;
    emit IncreaseReserve(msg.sender, _amount);
    }


## Assessed type

Reentrancy
c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

Picodes commented 1 year ago

Using this how do you drain the contract? Here you just showed that you could reenter in a state where _reserve is underestimated.