Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: medium
Valid

The function Dussehra killRavana can be called multiple times

[H-1] The function Dussehra::killRavana can be called multiple times, leading to organiser receiving all funds and leaving no funds for the selected ram to claim through the function Dussehra::withdraw.

Description: The function Dussehra::killRavana sets IsRavanKilled to true and sends half of the collected fees to the organiser address. The Dussehra::withdraw function, in turn, is meant to allow a winner address to withdraw the other half of the collected fees.

However, the killRavana function does not check if Ravana has already been killed (or, more generally, if the function has already been called before. It only checks if Ram has been selected (through the RamIsSelected modifier) and if it is called between block.timestamp 1728691069 and 1728691069. As a result, it can be called multiple times, each time transferring half of the collected fees to the organiser address.

function killRavana() public RamIsSelected {
if (block.timestamp < 1728691069) {
revert Dussehra__MahuratIsNotStart();
}
if (block.timestamp > 1728777669) {
revert Dussehra__MahuratIsFinished();
}
// A check if Ravana is already killed is missing here.
IsRavanKilled = true;

Impact: After two calls to the killRavana function, all funds have been sent to the organiser address, leaving none for the winner address to withdraw. It breaks intended functionality of the protocol and allows the organiser to execute a rug pull.

Proof of Concept:

  1. Participants enter the contract through the Dussehra::enterPeopleWhoLikeRam function.

  2. Each participant pays the entry fee.

  3. Between timestamp 1728691200 and 1728777600, the organiser calls the ChoosingRam::selectRamIfNotSelected function. This allows the killRavana function to be called.

  4. Any address calls the Dussehra::killRavana.

  5. A second time, any address calls the Dussehra::killRavana.

  6. All fees deposited into the protocol end up at organiser address.

Proof of Concept

Place the following in Dussehra.t.sol.

// note: the `participants` modifier adds two players to the protocol, both pay the 1 ether entree fee.
function test_organiserGetsAllFundsByCallingKillRavanaTwice() public participants {
uint256 balanceDussehraStart = address(dussehra).balance;
uint256 balanceOrganiserStart = organiser.balance;
vm.assertEq(balanceDussehraStart, 2 ether);
// the organiser first selects a Ram..
vm.warp(1728691200 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
vm.stopPrank();
// then the killRavana function is called twice.
vm.warp(1728691069 + 1);
vm.startPrank(player3);
// calling it one time...
dussehra.killRavana();
// calling it a second time... -- no revert happens.
dussehra.killRavana();
vm.stopPrank();
uint256 balanceDussehraEnd = address(dussehra).balance;
uint256 balanceOrganiserEnd = organiser.balance;
// The balance of Dussehra is 0 and the organiser took all the funds that were in the Dussehra contract.
vm.assertEq(balanceDussehraEnd, 0 ether);
vm.assertEq(balanceOrganiserEnd, balanceOrganiserStart + balanceDussehraStart);
// when withdraw is called it reverts: out of funds.
address selectedRam = choosingRam.selectedRam();
vm.startPrank(selectedRam);
vm.expectRevert();
dussehra.withdraw();
vm.stopPrank();
}

Recommended Mitigation: Add a check if Ravana has already been killed, making it impossible to call the function twice.

+ error Dussehra__RavanaAlreadyKilled()();
.
.
.
function killRavana() public RamIsSelected {
if (block.timestamp < 1728691069) {
revert Dussehra__MahuratIsNotStart();
}
if (block.timestamp > 1728777669) {
revert Dussehra__MahuratIsFinished();
}
+ if (IsRavanKilled) {
+ revert Dussehra__RavanaAlreadyKilled();
+ }
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.