Summary
This protocol implements time constraints for a number of functions. Dussehra::killRavana() is one such function, and it is of great important to RamNFT::organiser and ChoosingRam::selectedRam as it is the only way these two actors can receive their funds.
function killRavana() public RamIsSelected {
if (block.timestamp < 1728691069) {
revert Dussehra__MahuratIsNotStart();
}
if (block.timestamp > 1728777669) {
revert Dussehra__MahuratIsFinished();
}
...
}
Anyone can call this function, but it must be done in the span of one day
Of all the participants, only RamNFT::organiser and ChoosingRam::selectedRam are incentivized to call this function
It would be in the best interest of RamNFT::organiser and ChoosingRam::selectedRam to ensure that Dussehra::killRavana() is called within the given time constraints. If for any reason they are unable to call the function, all funds will be stuck in the contract.
Vulnerability Details
We can see this vulnerability in action using Foundry.
Foundry test setup
pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
import {Dussehra} from "../../src/Dussehra.sol";
import {ChoosingRam} from "../../src/ChoosingRam.sol";
import {RamNFT} from "../../src/RamNFT.sol";
contract CounterTest is Test {
error Dussehra__MahuratIsFinished();
Dussehra public dussehra;
RamNFT public ramNFT;
ChoosingRam public choosingRam;
address public organiser = makeAddr("organiser");
uint256 public constant PICK_A_NUMBER = 5;
address[] players = new address[](PICK_A_NUMBER);
function setUp() public {
vm.startPrank(organiser);
ramNFT = new RamNFT();
choosingRam = new ChoosingRam(address(ramNFT));
dussehra = new Dussehra(1 ether, address(choosingRam), address(ramNFT));
ramNFT.setChoosingRamContract(address(choosingRam));
vm.stopPrank();
}
modifier participants() {
for (uint256 i =0; i< PICK_A_NUMBER; i++) {
string memory stringNumber = vm.toString(i);
players[i] = makeAddr(stringNumber);
vm.deal(players[i], 1 ether);
vm.startPrank(players[i]);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
}
_;
}
}
Foundry Test
function test_stuckFunds() public participants {
uint256 contractBalance = address(dussehra).balance;
console.log("balance start: ", contractBalance);
vm.warp(1728691200 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
address selected = choosingRam.selectedRam();
vm.stopPrank();
vm.warp(1728777669+1);
vm.expectRevert(abi.encodeWithSelector(Dussehra__MahuratIsFinished.selector));
vm.startPrank(selected);
dussehra.killRavana();
vm.stopPrank();
vm.warp(1728777669+1);
vm.expectRevert(abi.encodeWithSelector(Dussehra__MahuratIsFinished.selector));
vm.startPrank(organiser);
dussehra.killRavana();
vm.stopPrank();
vm.expectRevert();
vm.startPrank(selected);
dussehra.withdraw();
vm.stopPrank();
contractBalance = address(dussehra).balance;
console.log("balance end: ", contractBalance);
assertEq(dussehra.IsRavanKilled(), false);
}
This test shows what happens if ChoosingRam::selectedRam or RamNFT::organiser miss the window to call Dussehra::killRavana().
We can see that the contract balance remains the same from the start of the test to the finish, and there is no way to extract these funds.
It is up to the protocol how to address this issue, but it should be addressed. This is an example of a simple fix added to Dussehra that would allow entrants to reclaim their entrance fees (this example neglects the RamNFT::organiser)
New Withdraw Option
function withdrawStillAlive() public {
if (IsRavanKilled) {
revert();
}
if (block.timestamp <= 1728777669) {
revert();
}
if (!peopleLikeRam[msg.sender]) {
revert();
}
peopleLikeRam[msg.sender] = false;
(bool success, ) = msg.sender.call{value: entranceFee}("");
if (!success) {
revert();
}
}
We need to make sure IsRavanKilled is false
The block.timestap has to be after the event is finished
msg.sender must return true when used in Dussehra::peopleLikeRam
Update msg.sender in Dussehra::peopleLikeRam to false
Allow msg.sender to recollect entrance fee
Foundry Test
function test_newWithdraw() public participants {
vm.warp(1728691200 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
address selected = choosingRam.selectedRam();
vm.stopPrank();
vm.warp(1728777669+1);
vm.expectRevert(abi.encodeWithSelector(Dussehra__MahuratIsFinished.selector));
vm.startPrank(selected);
dussehra.killRavana();
vm.stopPrank();
vm.warp(1728777669+1);
vm.expectRevert(abi.encodeWithSelector(Dussehra__MahuratIsFinished.selector));
vm.startPrank(organiser);
dussehra.killRavana();
vm.stopPrank();
uint256 contractBalance = address(dussehra).balance;
console.log("balance start: ", contractBalance);
vm.startPrank(players[0]);
dussehra.withdrawStillAlive();
vm.stopPrank();
vm.expectRevert();
vm.startPrank(players[0]);
dussehra.withdrawStillAlive();
vm.stopPrank();
contractBalance = address(dussehra).balance;
console.log("balance middle: ", contractBalance);
for (uint256 i=1; i<PICK_A_NUMBER;i++) {
vm.startPrank(players[i]);
dussehra.withdrawStillAlive();
vm.stopPrank();
}
contractBalance = address(dussehra).balance;
console.log("balance end: ", contractBalance);
assertEq(dussehra.IsRavanKilled(), false);
}
This test shows the implementation of Dussehra::withdrawStillAlive
After the event is over, if RamNFT::organiser and ChoosingRam::selectedRam fail to call Dussehra::killRavana(), the new withdraw function will allow participants to reclaim their entrance fees
The test shows that a participant can only call the function once
The contract balance at the end of the test is 0
Impact
This is a medium risk vulnerability that can lead to a loss of all funds if actors don't participate accordingly.
There are two participants that are incentivized to be mindful of the time constraints in Dussehra::killRavana (RamNFT::organiser and ChoosingRam::selectedRam). While anyone can call Dussehra::killRavana, it is up to two actors to make sure it happens within the time constrains. Dussehra::killRavana() is the only way RamNFT::organiser and ChoosingRam::selectedRam can receive their funds. So the likelihood of this vulnerability occurring intentionally is low. But if the vulnerability occurs unintentionally, the impact will lead to a complete loss of funds for both actors leaving the funds stuck in the contract.
Tools Used
Recommendations
I provided an example of a withdraw function that can be used to allow participants to reclaim their fees. That is just one way to fix the problem. But there are many ways to fix the issue, and the problem must be addressed.
The protocol may feel that RamNFT::organiser should still be rewarded as this was an nft mint. In that case the suggested function can be adjusted.
Alternative Fix
Add a bool to track whether the RamNFT::organiser has withdrawn:
contract Dussehra {
...
+ bool public organiserFee;
...
}
Add a calculation to Dussehra::withdrawStillAlive to return half the entrance fees:
function withdrawStillAlive() public {
if (IsRavanKilled) {
revert();
}
if (block.timestamp <= 1728777669) {
revert();
}
if (!peopleLikeRam[msg.sender]) {
revert();
}
peopleLikeRam[msg.sender] = false;
+ uint256 giveThemHalf = (entranceFee * 50) / 100;
+ (bool success, ) = msg.sender.call{value: giveThemHalf}("");
if (!success) {
revert();
}
}
Create Dussehra::organizerWithdraw()
function organizerWithdraw() public {
if (msg.sender != ramNFT.organiser()) {
revert();
}
if (address(this).balance == 0) {
revert();
}
if (organiserFee) {
revert();
}
organiserFee = true;
uint256 totalAmountByThePeople = WantToBeLikeRam.length * entranceFee;
totalAmountGivenToRam = (totalAmountByThePeople * 50) / 100;
(bool success, ) = organiser.call{value: totalAmountGivenToRam}("");
require(success, "Failed to send money to organiser");
}
Foundry Test
function test_giveThemHalf() public participants {
vm.warp(1728691200 + 1);
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
address selected = choosingRam.selectedRam();
vm.stopPrank();
uint256 contractBalance = address(dussehra).balance;
console.log("balance start: ", contractBalance);
vm.startPrank(organiser);
dussehra.organizerWithdraw();
vm.stopPrank();
vm.expectRevert();
vm.startPrank(organiser);
dussehra.organizerWithdraw();
vm.stopPrank();
contractBalance = address(dussehra).balance;
console.log("balance middle: ", contractBalance);
for (uint256 i=0; i<PICK_A_NUMBER;i++) {
vm.startPrank(players[i]);
dussehra.withdrawStillAlive();
vm.stopPrank();
}
contractBalance = address(dussehra).balance;
console.log("balance end: ", contractBalance);
assertEq(dussehra.IsRavanKilled(), false);
}
Another simple fix would be to remove the second time constraint in Dussehra::killRavana():
function killRavana() public RamIsSelected {
if (block.timestamp < 1728691069) {
revert Dussehra__MahuratIsNotStart();
}
- if (block.timestamp > 1728777669) {
- revert Dussehra__MahuratIsFinished();
- }
...
}
It is up to the protocol on how to proceed, but this is an issue that should not be left unaddressed.
