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

A Malicious `organiser` Can Obtain More Fees than They Are Entitled To, Effectively Draining the Contract

vile

Summary

Dussehra::killRavana does not implement a check to see if it has already been called, allowing a malicious organiser to call the function twice, draining the contract.

Vulnerability Details

A check does not exist in the Dussehra::killRavana function to see if IsRavanKilled is already set to true. This allows killRavana to be called repeatedly.

Add the following test case to Dussehra.t.sol:

Test Case
function test_ownerCanDrainContract() public participants {
// `organiser` calls `selectRamIfNotSelected` to set `isRamSelected` to `true`
vm.warp(1728777600);
vm.prank(organiser);
choosingRam.selectRamIfNotSelected();
​
// assert `isRamSelected`is set to `true`, and that the contract's balance is 2 ether
assertTrue(choosingRam.isRamSelected());
assertEq(address(dussehra).balance, 2 ether); // 1 ether (entranceFee) * 2
​
// call `killRavana` to pay the `organiser` their share, and to allow `withdraw` to be called
// anyone can call this function (e.g., the organiser, or an alternative wallet of the organiser), but we prank for clarity
vm.prank(organiser);
dussehra.killRavana();
​
// assert that:
// - the organiser has taken 50% (1 ether) of the funds
// - 50% remains for the `selectedRam`
// - that `IsRavanKilled` has been set to `true` by the pervious call
assertEq(address(dussehra).balance, 1 ether);
assertEq(address(organiser).balance, 1 ether);
assertTrue(dussehra.IsRavanKilled());
​
// the `organiser` is able to call `killRavana` once again
// anyone can call this function (e.g., the organiser, or an alternative wallet of the organiser), but we prank for clarity
vm.prank(organiser);
dussehra.killRavana();
​
// assert that the `organiser` has claimed the other 50% of the funds
assertEq(address(dussehra).balance, 0 ether);
assertEq(address(organiser).balance, 2 ether);
​
// now, the `selectedRam` is unable to withdraw (there is not enough ether in the contract)
assertEq(choosingRam.selectedRam(), address(player2));
vm.prank(player2);
vm.expectRevert("Failed to send money to Ram");
dussehra.withdraw();
}

Then, run the test:

forge test --mt test_ownerCanDrainContract -vvvvv

Impact

Since killRavana can be called multiple times, it means that the organiser can receive more ETH than they are entitled to.

For example, say 100 people have entered the event and all paid an entranceFee of 1 ether. 100 ether is in the contract, 50 ether is intended for the organiser and the other 50 ether is intended for the selectedRam. Once ChoosingRam::selectRamIfNotSelected has been called, a subsequent killRavana is required. The following scenario is possible:

On the first call:

  • totalAmountByThePeople = 100 (WantToBeLikeRam.length) * 1 ether (entranceFee) = 100 ether

  • totalAmountGivenToRam = (100 ether * 50) / 100 = 50 ether

  • organiser is then sent totalAmountGivenToRam (50) ether

  • 50 ether remains in the contract

Since no check exists to see if this has already happened, during a second call:

  • totalAmountByThePeople = 100 (WantToBeLikeRam.length) * 1 ether (entranceFee) = 100 ether

  • totalAmountGivenToRam = (100 ether * 50) / 100 = 50 ether

  • organiser is then sent totalAmountGivenToRam (50) ether

  • 0 ether remains in the contract

Now, when the selectedRam calls withdraw, there is not enough ether in the contract to fulfill the withdrawal.

Tools Used

Manual Review

Recommendations

killRavana should implement a check to see if IsRavanKilled is already set to true.

+ modifier RavanNotKilled() {
+ require(!IsRavanKilled, "Ravan has already been killed!");
+ _;
+ }
​
// ...
​
- function killRavana() public RamIsSelected {
+ function killRavana() public RamIsSelected RavanNotKilled {
// ...
IsRavanKilled = true;
// ...
}
Updates

Lead Judging Commences

bube Lead Judge
about 1 year ago
bube Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

isRavanKilled is not checked

Support

FAQs

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