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

### [H-2] `ChoosingRam::isRamSelected` bool is not updated in the `increaseValuesOfParticipants` function causes inconsistent state.

Keyword

[H-2] ChoosingRam::isRamSelected bool is not updated in the increaseValuesOfParticipants function. Because of this, Ram can never call Dussehra::killRavana or Dussehra::withdraw functions and the organiser will overwrite the address of selectedRam with a new value.

Description: By calling the ChoosingRam::increaseValuesOfParticipants function, users can change the characteristics of their NFTs. When an NFT has all the bools in the RamNFT::CharacteristicsOfRam set to true, the address of the owner of that NFT is designated as the ChoosingRam::selectedRam.

function increaseValuesOfParticipants(...) public RamIsNotSelected {
//..
selectedRam = ramNFT.getCharacteristics(tokenIdOfChallenger).ram;
//..
}

The problem here is that this function does not set the bool public isRamSelected; to true once this happens, therefore the contract's state is inconsistent.

Impact: A winner that has his address set as the selectedRam won't be able to call the Dussehra::killRavana or Dussehra::withdraw functions to claim his prize because the RamIsSelected modifier will always revert. Furthermore, once block.timestamp will exceed 1728691200, the organiser is able to call the ChoosingRam::selectRamIfNotSelected function, which will override the value of selectedRam.

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

PoC - Click the arrow below
function test_boolNotUpdated() 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();
​
//make NFT of player2 `selectedRam`
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(ramNFT.getCharacteristics(1).isSatyavaakyah, true);
address chosenRam = choosingRam.selectedRam();
//player2 is selectedRam because we updated his NFTs characteristics at index1 above
​
//player3 joins event
vm.startPrank(player3);
vm.deal(player3, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
//player2 tries to killRavana and withdraw, these will revert because of the `RamIsSelected` modifier
vm.warp(1728691200 + 1);
vm.startPrank(player2);
vm.expectRevert();
dussehra.killRavana();
vm.expectRevert();
dussehra.withdraw();
vm.stopPrank();
​
//organiser selects a new Ram and overwrite player2's address with player 3's address
vm.prank(organiser);
choosingRam.selectRamIfNotSelected();
address chosenRam2 = choosingRam.selectedRam();
assertNotEq(chosenRam, chosenRam2);
}

Test output

[PASS] test_boolNotUpdated() (gas: 576923)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.79ms (708.15µs CPU time)
Ran 1 test suite in 6.71ms (1.79ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended mitigation: Update the isRamSelected bool inside the ChoosingRam::increaseValuesOfParticipants when the address of selectedRam is set.

function increaseValuesOfParticipants(...) public RamIsNotSelected {
//..
//..
if (random == 0) {
//..
//..
selectedRam = ramNFT.getCharacteristics(tokenIdOfChallenger).ram;
+ isRamSelected = true;
}
} else {
//..
//..
selectedRam = ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).ram;
+ isRamSelected = true;
}
}
}
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.