First Flight #17: Dussehra

First Flight #17

Beginner FriendlyFoundryNFT

100 EXP

View results

Previous Next

Submission Details

Severity: medium

Valid

Lack of restrictions on `tokenIdOfAnyParticipant` allows any user to select Ram by repeatedly calling `ChoosingRam::increaseValuesOfParticipants` with the right arguments.

feder

Summary

Any user can select Ram after calling increaseValuesOfParticipants 5 times, passing the same tokenId for both the challenger and the "participant." A similar result can be achieved by passing two distinct tokens, both belonging to the caller, and calling the function 9 times. As a result, the caller can select Ram and access a monetary reward.

Vulnerability Details

It is not clear from the specification, but it seems like the idea behind the increaseValuesOfParticipants function is to allow 2 distinct tokens to "battle" against each other and provide the winner with a boost in experience. Once a token wins 5 battles, it can select Ram and access a reward.

The issue originates from the fact that the user is allowed to choose the "participant" token without any restrictions. Then, if the user enters the same token for both parameters, a "win" is assured and a boost in experience is applied to the provided token. By repeating this process 5 times, the user can select Ram and later on withdraw a reward.

function increaseValuesOfParticipants(uint256 tokenIdOfChallenger, uint256 tokenIdOfAnyParticipant)

public

RamIsNotSelected

{

if (tokenIdOfChallenger > ramNFT.tokenCounter()) {

revert ChoosingRam__InvalidTokenIdOfChallenger();

}

if (tokenIdOfAnyParticipant > ramNFT.tokenCounter()) {

revert ChoosingRam__InvalidTokenIdOfParticipant();

}

if (ramNFT.getCharacteristics(tokenIdOfChallenger).ram != msg.sender) {

revert ChoosingRam__CallerIsNotChallenger();

}

if (block.timestamp > 1728691200) {

revert ChoosingRam__TimeToBeLikeRamFinish();

}

uint256 random =

uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % 2;

// @audit providing the same tokenId for both parameters ensures that the NFT gets updated

// regardless of the random result

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; // @audit can we make this 0x?

}

} else {

if (ramNFT.getCharacteristics(tokenIdOfAnyParticipant).isJitaKrodhah == false){

ramNFT.updateCharacteristics(tokenIdOfAnyParticipant, true, false, false, false, false);

} else if (ramNFT.getCharacteristics(tokenIdOfAnyParticipant).isDhyutimaan == false){

ramNFT.updateCharacteristics(tokenIdOfAnyParticipant, true, true, false, false, false);

} else if (ramNFT.getCharacteristics(tokenIdOfAnyParticipant).isVidvaan == false){

ramNFT.updateCharacteristics(tokenIdOfAnyParticipant, true, true, true, false, false);

} else if (ramNFT.getCharacteristics(tokenIdOfAnyParticipant).isAatmavan == false){

ramNFT.updateCharacteristics(tokenIdOfAnyParticipant, true, true, true, true, false);

} else if (ramNFT.getCharacteristics(tokenIdOfAnyParticipant).isSatyavaakyah == false){

ramNFT.updateCharacteristics(tokenIdOfAnyParticipant, true, true, true, true, true);

selectedRam = ramNFT.getCharacteristics(tokenIdOfAnyParticipant).ram;

}

PoC

Add the following test to the test suite, which shows that player1 can select Ram after calling increaseValuesOfParticipants 5 times with the same tokenId for both parameters.

function test_exploitIncreaseValuesToSelectRam() public participants {

vm.startPrank(player1);

choosingRam.increaseValuesOfParticipants(0, 0);

vm.stopPrank();

assertEq(choosingRam.selectedRam(), player1);

}

Impact

Any user would be able to select Ram and access the reward, which makes the protocol meaningless.

Tools Used

Manual review.

Recommended Mitigation

Instead of allowing the user to pass any arbitrary tokenId for the second parameter (tokenIdOfAnyParticipant), choose a token at random (excluding the provided token by the user). In this manner, you ensure a fair "battle" and reduce the chances of manipulation.

- function increaseValuesOfParticipants(uint256 tokenIdOfChallenger, uint256 tokenIdOfAnyPerticipent)

+ function increaseValuesOfParticipants(uint256 tokenIdOfChallenger)

public

RamIsNotSelected

{

if (tokenIdOfChallenger > ramNFT.tokenCounter()) {

revert ChoosingRam__InvalidTokenIdOfChallenger();

}

if (tokenIdOfAnyParticipant > ramNFT.tokenCounter()) {

revert ChoosingRam__InvalidTokenIdOfParticipant();

}

if (ramNFT.getCharacteristics(tokenIdOfChallenger).ram != msg.sender) {

revert ChoosingRam__CallerIsNotChallenger();

}

if (block.timestamp > 1728691200) {

revert ChoosingRam__TimeToBeLikeRamFinish();

}

// Randomly generate a second tokenId, making sure it is different than the challenger tokenId

+ uint256 rand = uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % (ramNFT.tokenCounter() - 1);

// ^ rand in [0, ..., tokenCounter - 2]

+ uint256 tokenIdOfAnyParticipant = (tokenIdOfChallenger + 1 + rand) % ramNFT.tokenCounter();

// ^ The only way for tokenIdOfAnyParticipant to equal tokenIdOfChallenger is if

// (1 + rand) === 0 OR (1 + rand) === tokenCounter, but that never holds because:

// (1 + rand) is in [1, ..., tokenCounter - 1]