Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: medium
Invalid

`Game::withdrawWinnings function` not following CEI method

anuragshingare2005

Game::withdrawWinnings function does not follow CEI method

Description

  • Game::withdrawWinnings function implments nonReentrant modifier, but CEI method is not implemented properly. The current code in declareWinner function is standard way for reentrancy attack

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
@> (bool success, ) = payable(msg.sender).call{value: amount}("");
@> require(success, "Game: Failed to withdraw winnings.");
@> pendingWinnings[msg.sender] = 0;
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Proof of Concept

  • Run the command forge test --mt test_reentrancy_protection_works_but_CEI_violated -vvvv to check the test result

  • The test is created just to proove that, function implements nonReentrant modifier still does not follow CEI method. Reentrancy is still possible which will eventually fail.

contract MaliciousReentrancy {
Game public game;
uint256 public attackCount;
constructor(Game _game) {
game = _game;
}
​
receive() external payable {
attackCount++;
if (attackCount < 3 && game.pendingWinnings(address(this)) > 0) {
game.withdrawWinnings(); // This would fail due to nonReentrant modifier
}
}
function attack() external {
game.withdrawWinnings();
}
}
function test_reentrancy_protection_works_but_CEI_violated() public {
MaliciousReentrancy malicious = new MaliciousReentrancy(game);
vm.deal(address(malicious), 10 ether);
vm.startPrank(address(malicious));
game.claimThrone{value: 2 ether}();
vm.stopPrank();
uint256 graceTime = game.gracePeriod();
vm.warp(block.timestamp + graceTime + 1);
vm.startPrank(user2);
game.declareWinner();
vm.stopPrank();
uint256 winnings = game.pendingWinnings(address(malicious));
assert(winnings > 0);
vm.startPrank(address(malicious));
// The receive function will be called and try to reenter, causing failure
vm.expectRevert("Game: Failed to withdraw winnings.");
malicious.attack();
vm.stopPrank();
}
​

Recommended Mitigation

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
+ pendingWinnings[msg.sender] = 0;
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
- pendingWinnings[msg.sender] = 0;
emit WinningsWithdrawn(msg.sender, amount);
}
Updates

Appeal created

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Game::withdrawWinnings reentrancy

We only audit the current code in scope. We cannot make speculation with respect to how this codebase will evolve in the future. For now there is a nonReentrant modifier which mitigates any reentrancy. CEI is a good practice, but it's not mandatory. Informational

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.