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

The ChoosingRam::selectRamIfNotSelected always overrides selectedRam before the end of the event.

7Cedars

[H-5] The ChoosingRam::increaseValuesOfParticipants does not set isRamSelected to true. It results in the ChoosingRam::selectRamIfNotSelected overriding any prior selected Ram before the end of the event.

Description: The ChoosingRam::increaseValuesOfParticipants function is meant as a game of chance between two participants (a tokenIdOfChallenger and tokenIdOfAnyPerticipent). One of the two receives an increase in characteristics. If enough characteristics have been accumulated, the participant will be selected as the Ram and win half of the fee pool. An additional function ChoosingRam::selectRamIfNotSelected allows the organiser to select a Ram if none has been selected by a certain time.

However, the ChoosingRam::increaseValuesOfParticipants does not set isRamSelected to true when it selects a Ram. As a result:

  1. increaseValuesOfParticipants can continue to select a Ram even if it has already been selected.

  2. selectRamIfNotSelected can overwrite any Ram selected through increaseValuesOfParticipants.

  3. Worse, because the Dussehra::killRavana function checks if ChoosingRam::isRamSelected is true, it forces ChoosingRam::selectRamIfNotSelected to be called. This means that the selected Ram will always be set by the selectRamIfNotSelected function, not the increaseValuesOfParticipants.

In ChoosingRam.sol:

function increaseValuesOfParticipants(uint256 tokenIdOfChallenger, uint256 tokenIdOfAnyPerticipent)
.
.
.
} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isSatyavaakyah == false){
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, true, true, true);
// Note: isRamSelected not set to true
selectedRam = ramNFT.getCharacteristics(tokenIdOfChallenger).ram;
}
.
.
.
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isSatyavaakyah == false){
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, true, true, true);
// Again note: isRamSelected not set to true
selectedRam = ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).ram;
}
​

In Dussehra.sol:

function killRavana() public RamIsSelected {

Impact: The intended functionality of the protocol is for participants to increase their characteristics through the ChoosingRam::increaseValuesOfParticipants function until they become Ram. Only in the case that no one has been selected as Ram though increaseValuesOfParticipants, does the organiser get to randomly select a Ram. This bug in the protocol breaks its intended logic.

Proof of Concept:

  1. Two participants (player1 and player2) call increaseValuesOfParticipants until one is selected as Ram.

  2. When Dussehra::killRavana is called, it reverts.

  3. When organiser calls selectRamIfNotSelected it does not revert.

  4. The selectedRam is reset to a new address.

  5. When Dussehra::killRavana is called, it does not revert.

Proof of Concept

Place the following in the CounterTest contract of the Dussehra.t.sol test file.

function test_selectRamIfNotSelected_AlwaysSelectsRam() public participants {
address selectedRam;
// the organiser enters the protocol, in additional to player1 and player2.
vm.startPrank(organiser);
vm.deal(organiser, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
// check that the organiser owns token id 2:
assertEq(ramNFT.ownerOf(2), organiser);
​
// player1 and player2 play increaseValuesOfParticipants against each other until one is selected.
vm.startPrank(player1);
while (selectedRam == address(0)) {
choosingRam.increaseValuesOfParticipants(0, 1);
selectedRam = choosingRam.selectedRam();
}
// check that selectedRam is player1 or player2:
assert(selectedRam== player1 || selectedRam == player2);
// But when calling Dussehra.killRavana(), it reverts because isRamSelected has not been set to true.
vm.expectRevert("Ram is not selected yet!");
dussehra.killRavana();
vm.stopPrank();
​
// Let the organiser predict when their own token will be selected through the (not so) random selectRamIfNotSelected function.
uint256 j;
uint256 calculatedId;
while (calculatedId != 2) {
j++;
vm.warp(1728691200 + j);
calculatedId = uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % ramNFT.tokenCounter();
}
// when the desired id comes up, the organiser calls `selectRamIfNotSelected`:
vm.startPrank(organiser);
choosingRam.selectRamIfNotSelected();
vm.stopPrank();
selectedRam = choosingRam.selectedRam();
​
// check that selectedRam is now the organiser:
assert(selectedRam == organiser);
// and we can call killRavana() without reverting:
dussehra.killRavana();
}

Recommended Mitigation: The simplest mitigation is to set isRamSelected to true when a ram is selected through the increaseValuesOfParticipants function.

} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isSatyavaakyah == false){
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, true, true, true);
+ isRamSelected = true;
selectedRam = ramNFT.getCharacteristics(tokenIdOfChallenger).ram;
}
.
.
.
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isSatyavaakyah == false){
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, true, true, true);
+ isRamSelected = true;
selectedRam = ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).ram;
}

Please note that another mitigation would be to delete the isRamSelected state variable altogether and have the RamIsNotSelected modifier check if selectedRam != address(0). This simplifies the code and reduces chances of errors. This does necessity additional changes to the Dussehra.sol contract.

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.

Give us feedback!