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

`ChoosingRam::increaseValuesOfParticipants` allows the challenger tokenId and the participant tokenId to be the same, resulting in the calling player's characteristics always increasing.

poolig4n

Description

A malicious player can manipulate the event's outcome by calling the function ChoosingRam::increaseValuesOfParticipants with their own tokenId as both the challenger and the participant resulting in the increase in values each call.

Impact

Manipulation of the contract's game mechanics leading to an unfair advantage by a malicious player.

Proof of Concept

Standalone Test

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {Dussehra} from "../src/Dussehra.sol";
import {ChoosingRam} from "../src/ChoosingRam.sol";
import { mock } from "../src/mocks/mock.sol";
import {RamNFT} from "../src/RamNFT.sol";
​
contract IncreaseValueTest is Test {
// error Dussehra__NotEqualToEntranceFee();
// error Dussehra__AlreadyClaimedAmount();
// error ChoosingRam__TimeToBeLikeRamIsNotFinish();
// error ChoosingRam__EventIsFinished();
​
Dussehra public dussehra;
RamNFT public ramNFT;
ChoosingRam public choosingRam;
address public organiser = makeAddr("organiser");
address public player1 = makeAddr("player1");
address public player2 = makeAddr("player2");
address public player3 = makeAddr("player3");
address public player4 = makeAddr("player4");
address public player5 = makeAddr("player5");
​
modifier participants() {
vm.startPrank(player1);
vm.deal(player1, 1 ether);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
_;
}
function setUp() public {
vm.startPrank(organiser);
ramNFT = new RamNFT();
choosingRam = new ChoosingRam(address(ramNFT));
dussehra = new Dussehra(1 ether, address(choosingRam), address(ramNFT));
​
ramNFT.setChoosingRamContract(address(choosingRam));
vm.stopPrank();
}
​
​
// It's possible to for challenger and participant token to be the same.
// This results in always increasing the values of the address and therefore
// 'games the system' for selecting Ram as a player.
// Recommend: checking that challenger and participant tokens are not equal
// Severity: Medium
function test_increaseValuesOfParticipantSamePlayer() public participants {
vm.startPrank(player1);
choosingRam.increaseValuesOfParticipants(0, 0);
choosingRam.increaseValuesOfParticipants(0, 0);
choosingRam.increaseValuesOfParticipants(0, 0);
choosingRam.increaseValuesOfParticipants(0, 0);
choosingRam.increaseValuesOfParticipants(0, 0);
vm.stopPrank();
assertEq(ramNFT.getCharacteristics(0).isJitaKrodhah, true);
assertEq(ramNFT.getCharacteristics(0).isDhyutimaan, true);
assertEq(ramNFT.getCharacteristics(0).isVidvaan, true);
assertEq(ramNFT.getCharacteristics(0).isAatmavan, true);
assertEq(ramNFT.getCharacteristics(0).isSatyavaakyah, true);
​
//selectedRam is set
assertEq(choosingRam.selectedRam(), ramNFT.getCharacteristics(0).ram );
}
}

Test Result

forge test --mt test_increaseValuesOfParticipantSamePlayer -vv
[⠊] Compiling...
[⠒] Compiling 1 files with 0.8.20
[⠑] Solc 0.8.20 finished in 4.55s
Compiler run successful!
​
Ran 1 test for test/IncreaseValueTest.t.sol:IncreaseValueTest
[PASS] test_increaseValuesOfParticipantSamePlayer() (gas: 310740)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 21.11ms (2.43ms CPU time)
​
Ran 1 test suite in 451.62ms (21.11ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended mitigation

  • Add a check in the function function increaseValuesOfParticipants(uint256 tokenIdOfChallenger, uint256 tokenIdOfAnyPerticipent) to ensure that tokenIdOfChallenger and tokenIdOfAnyPerticipent are not equal.

The code below is indicative of one possible solution and must not be considered as production ready code.

function increaseValuesOfParticipants(uint256 tokenIdOfChallenger,
uint256 tokenIdOfAnyPerticipent)
public
RamIsNotSelected {
+ if (tokenIdOfChallenger == tokenIdOfAnyPerticipent) {
+ revert ChoosingRam__InvalidTokenIdOfPerticipent();
+ }
//rest of function ommitted ..

References
https://github.com/Cyfrin/2024-06-Dussehra/blob/9c86e1b09ed9516bfbb3851c145929806da75d87/src/ChoosingRam.sol#L33

Tools Used

  • Manual Review

Updates

Lead Judging Commences

bube Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Challenge themselves

Support

FAQs

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