Summary
When ram is selected with ChoosingRam.sol::increaseValuesOfParticipants, ChoosingRam.sol::isRamSelected remains false meaning ram can be overriden and ram cannot withdraw the funds that theyve won.
Vulnerability Details
ChoosingRam.sol::isRamSelected is not set to true when ChoosingRam.sol::selectedRam is set when all nft traits are true. So, it is still thought that ram has not been selected as the boolean is still false.
Proof of code imapct 1.
function test_canOverrideRam() public participants {
uint256 timeStamp = 1722470400;
uint256 prevrandao;
uint256 player1TokenId = 0;
uint256 player2TokenId = 1;
for (uint256 i = 0; i < 100; i++) {
vm.prevrandao(bytes32(i));
if (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, player1))) % 2 == 0) {
prevrandao = i;
break;
}
}
vm.startPrank(player1);
for (uint256 i = 0; i < 5; i++) {
choosingRam.increaseValuesOfParticipants(player1TokenId, player2TokenId);
}
vm.stopPrank();
assertEq(choosingRam.selectedRam(), player1);
for (uint256 i = 0; i < 100; i++) {
vm.prevrandao(bytes32(i));
if (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, player2))) % 2 == 0) {
prevrandao = i;
break;
}
}
vm.startPrank(player2);
for (uint256 i = 0; i < 5; i++) {
choosingRam.increaseValuesOfParticipants(player2TokenId, player1TokenId);
}
vm.stopPrank();
assertEq(choosingRam.selectedRam(), player2);
}
Player2 becomes ram after player1 was ram
Proof of code impact 2.:
function test_isRamSelectedNotUpdated() public participants {
vm.startPrank(player3);
vm.deal(player3, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
uint256 timeStamp = 1722470400;
uint256 prevrandao;
uint256 player1TokenId = 0;
uint256 player2TokenId = 1;
for (uint256 i = 0; i < 100; i++) {
vm.prevrandao(bytes32(i));
if (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, player1))) % 2 == 0) {
prevrandao = i;
break;
}
}
vm.startPrank(player1);
for (uint256 i = 0; i < 5; i++) {
choosingRam.increaseValuesOfParticipants(player1TokenId, player2TokenId);
}
vm.stopPrank();
assertEq(choosingRam.selectedRam(), player1);
assertFalse(choosingRam.isRamSelected());
vm.expectRevert();
dussehra.killRavana();
vm.startPrank(player1);
vm.expectRevert();
dussehra.withdraw();
vm.stopPrank();
}
player1 cant withdraw even though they are selected ram.
Impact
ChoosingRam.sol::RamIsNotSelected modifier on ChoosingRam.sol::increaseValuesOfParticipants doesn't prevent another address from replacing the old selected ram if the previous ram was set by ChoosingRam.sol::increaseValuesOfParticipants.
The address who made became ram first through ChoosingRam.sol::increaseValuesOfParticipants cannot withdraw the eth they earned. If someone else uses impact 1. and overrides the previous ram, they too can also not withdraw. This is because of the modifier Dussehra.sol::RamIsSelected on Dussehra.sol::withdraw depends on the boolean ChoosingRam.sol::isRamSelected which is false if ram is chosen through ChoosingRam.sol::increaseValuesOfParticipants.
Tools Used
Manual review
Recommendations
set ChoosingRam.sol::isRamSelected when setting selected ram.
if (random == 0) {
if (ramNFT.getCharacteristics(tokenIdOfChallenger).isJitaKrodhah == false) {
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, false, false, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isDhyutimaan == false) {
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, false, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isVidvaan == false) {
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, true, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isAatmavan == false) {
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, true, true, false);
} else if (ramNFT.getCharacteristics(tokenIdOfChallenger).isSatyavaakyah == false) {
ramNFT.updateCharacteristics(tokenIdOfChallenger, true, true, true, true, true);
selectedRam = ramNFT.getCharacteristics(tokenIdOfChallenger).ram;
+ isRamSelected = true;
}
} else {
if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isJitaKrodhah == false) {
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, false, false, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isDhyutimaan == false) {
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, false, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isVidvaan == false) {
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, true, false, false);
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isAatmavan == false) {
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, true, true, false);
} else if (ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).isSatyavaakyah == false) {
ramNFT.updateCharacteristics(tokenIdOfAnyPerticipent, true, true, true, true, true);
selectedRam = ramNFT.getCharacteristics(tokenIdOfAnyPerticipent).ram;
+ isRamSelected = true;
}
}
Extra recommendation:
create a method that handles all the ifs and else ifs to avoid duplicate code.
