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

Withdrawals from `Dussehra::withdraw` and `Dussehra::killRavana` functionality are blocked when `selectedRam` is set by calling `ChoosingRam::increaseValuesOfParticipants`

appet

Description:

When OnlyRam (a.k.a. the winning user) has been previously set selectedRam by calling ChoosingRam::increaseValuesOfParticipants, tries to kills Ravana (calling Dussehra::killRavana) and then calls Dussehra::withdraw function to collect his prize, the both functions will revert due to RamIsSelected modifier not passing the stated requirement.

Impact:

Calling Dussehra::killRavana by anyone is impossible and also withdrawals by OnlyRam when he is set selectedRam by calling ChoosingRam::increaseValuesOfParticipants, which is the primary way of picking a competitor for the prize.
This could lead to major problems -i.e. no allocation functionality of the Organiser`s or the winner prize which leads to locked funds in the contract.

Proof of Concept:

Paste the following code in the Dussehra.t.sol and initiate forge test --mt test_withdrawWhenSetRamOwn -vvvvv to see reason for revert: Ram is not selected yet!

​
function test_withdrawWhenSetRamOwn () public participants {
​
vm.startPrank(player1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
vm.stopPrank();
​
assertEq(choosingRam.selectedRam(), player2);
assertEq(choosingRam.isRamSelected(), false);
vm.expectRevert();
vm.startPrank(player2);
dussehra.killRavana();
vm.stopPrank();
​
vm.expectRevert();
vm.startPrank(player2);
dussehra.withdraw();
vm.stopPrank();
}
​

Recommended Mitigation:

Although there is a way of changing the isRamSelected bool to true by the Organiser calling ChoosingRam::selectRamIfNotSelected function, this is not the primary way of setting selectedRam. As per the specs, this should be done ONLY IF not selected by the user.
Consider adding a line of code that changes the isRamSelected bool to true in ChoosingRam::increaseValuesOfParticipants

​
function increaseValuesOfParticipants(uint256 tokenIdOfChallenger, uint256 tokenIdOfAnyPerticipent)
public
RamIsNotSelected
{
.
.
.
+ isRamSelected = true;
}

Tools Used

Manual review, Foundry

Updates

Lead Judging Commences

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

`isRamSelected` is not set

Support

FAQs

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