First Flight #20: The Predicter

Summary

A critical vulnerability has been discovered in the ThePredicter smart contract that allows a re-entrancy attack. This vulnerability occurs in the cancelRegistration function, where an external call to transfer Ether back to the user is made before updating the user's status, potentially allowing the attacker to recursively call the function and withdraw funds multiple times.

Vulnerability Details

The withdrawPredictionFees and withdraw functions in ThePredicter.sol are susceptible to underflow errors under certain conditions, such as when the organizer calls the withdraw function before withdrawPredictionFees. This could prevent the organizer from withdrawing the contract's balance. Introducing state variables for allPredictionFee and allEntranceFee and implementing additional checks can mitigate this risk.

https://github.com/Cyfrin/2024-07-the-predicter/blob/839bfa56fe0066e7f5610197a6b670c26a4c0879/src/ThePredicter.sol#L106

https://github.com/Cyfrin/2024-07-the-predicter/blob/839bfa56fe0066e7f5610197a6b670c26a4c0879/src/ThePredicter.sol#L141

Proof of Concept

This is the test code of reentrancy attack.

To test this code:

• Input this code to new test solidity file: test/CheckWithdrawPredictionFees.test.sol.

• Then run this command:

  forge test --match-path test/CheckWithdrawPredictionFees.test.sol --match-test testWithdraw -vvvv

• The result is:

  ├─ emit log_named_uint(key: "stranger.balance", val: 959700000000000000 [9.597e17])

  ├─ emit log_named_uint(key: "predicter.balance", val: 40300000000000000 [4.03e16])

  ├─ [0] VM::startPrank(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC])

  │ └─ ← [Return]

  ├─ [28464] ThePredicter::withdraw()

  │ ├─ [993] ScoreBoard::isEligibleForReward(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC]) [staticcall]

  │ │ └─ ← [Return] true

  │ ├─ [8025] ScoreBoard::getPlayerScore(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC]) [staticcall]

  │ │ └─ ← [Return] 3

  │ ├─ [8025] ScoreBoard::getPlayerScore(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC]) [staticcall]

  │ │ └─ ← [Return] 3

  │ ├─ [751] ScoreBoard::clearPredictionsCount(stranger: [0x49052147F5D97A723DEBdf07680fFFaDAd29A5dC])

  │ │ └─ ← [Stop]

  │ ├─ [0] stranger::fallback{value: 40000000000000000}()

  │ │ └─ ← [Stop]

  │ └─ ← [Stop]

  ├─ [0] VM::stopPrank()

  │ └─ ← [Return]

  ├─ emit log_named_uint(key: "stranger.balance", val: 999700000000000000 [9.997e17])

  ├─ emit log_named_uint(key: "predicter.balance", val: 300000000000000 [3e14])

  ├─ [0] VM::expectRevert(custom error f4844814:)

  │ └─ ← [Return]

  ├─ [0] VM::startPrank(organizer: [0xEA76a77949a33b9456E691c4c137B047B09037a0])

  │ └─ ← [Return]

  ├─ [699] ThePredicter::withdrawPredictionFees()

  │ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)

  ├─ [0] VM::stopPrank()

  │ └─ ← [Return]

As you can see, arithmetic underflow or overflow error occurs and organizer can't withdraw the contract's balance.

Impact

• Underflow errors can result in incorrect calculations, leading to potential failures in fee withdrawal and reward distribution.

• The current implementation may prevent the organizer from withdrawing the contract's balance under specific conditions, impacting the contract's functionality.

Tools Used

Manual code review

Recommendations

• Define allPredictionFee and allEntranceFee as state variables to ensure consistency and facilitate validation checks.

  uint256 public allPredictionFee;

  uint256 public allEntranceFee;

• Modify constructor.

  allPredictionFee = 0;

  allEntranceFee = 0;

• Modify register function.

  allEntranceFee += entranceFee;

• Modify cancelRegistration function.

  allEntranceFee -= entranceFee;

• Modify makePrediction function.

  allPredictionFee += predictionFee;

• Modify withdrawPredictionFees function.

  require(address(this).balance >= allPredictionFee, "Insufficient balance to cover prediction fee");

  (bool success, ) = msg.sender.call{value: allPredictionFee}("");

• Modify withdraw function.

  reward = maxScore < 0

  ? entranceFee

  : (shares * allEntranceFee) / totalShares;

  ​

  require(address(this).balance >= reward, "Insufficient balance to cover reward");