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

The function `ChoosingRam::increaseValuesOfParticipants` allows `selectedRam` to be set to an uninitialised `CharacteristicsOfRam` struct resulting in funds being stuck.

poolig4n

Description

The implementation of ChoosingRam::increaseValuesOfParticipants allows selectedRam to be set to an uninitialised CharacteristicsOfRam struct with the default address of address(0).
This would result in funds being trapped as the ability to withdraw funds is restricted by the OnlyRam modifier on the withdraw function in the Dussehra contract.

Relevant code snippet diff: ChoosingRam::increaseValuesOfParticipants

-if (tokenIdOfChallenger > ramNFT.tokenCounter()) {
+if (tokenIdOfChallenger >= ramNFT.tokenCounter()) {
revert ChoosingRam__InvalidTokenIdOfChallenger();
}
-if (tokenIdOfAnyPerticipent > ramNFT.tokenCounter()) {
+if (tokenIdOfAnyPerticipent >= ramNFT.tokenCounter()) {
revert ChoosingRam__InvalidTokenIdOfPerticipent();
}

The relevant code from the Dussehra.sol contract.

modifier OnlyRam() {
require(choosingRamContract.selectedRam() == msg.sender,
"Only Ram can call this function!");
_;
}
​
function withdraw() public RamIsSelected OnlyRam RavanKilled {
//function body ommitted...

Impact
Funds at risk if selectedRam is equal to address(0) as the withdraw function uses the OnlyRam modifier to check msg.sender is equal to selectedRam.

Proof of Concept

Standalone Test shown below

​
// 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();
}
​
// Audit tests
​
// it's possible for selectedRam to be set to address(0)
// Funds at risk if unitialised player becomes Ram.
// Severity: High
// Recommend: Fixing the > test to be >=
function test_increaseValuesOfParticipantSinglePlayer()
public participants {
// there is only one player so far; index 0,
// index 1 is an empty player / uninitialised player
// with address(0) default value as the struct's ram property.
vm.startPrank(player1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
choosingRam.increaseValuesOfParticipants(0, 1);
vm.stopPrank();
assertEq(ramNFT.getCharacteristics(1).isJitaKrodhah, true);
assertEq(ramNFT.getCharacteristics(1).isDhyutimaan, true);
assertEq(ramNFT.getCharacteristics(1).isVidvaan, true);
assertEq(ramNFT.getCharacteristics(1).isAatmavan, true);
assertEq(ramNFT.getCharacteristics(1).isSatyavaakyah, true);
​
// selected Ram is a non-player and the address is address(0)
// so the pct of funds for the winner would be stuck.
// as withdraw uses the OnlyRam modifier which retricts the caller to
// choosingRam.selectedRam
assertEq(choosingRam.selectedRam(), ramNFT.getCharacteristics(1).ram );
assertEq(choosingRam.selectedRam(), address(0) );
}
}

Test Result

forge test --mt test_increaseValuesOfParticipantSinglePlayer -vv
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/IncreaseValueTest.t.sol:IncreaseValueTest
[PASS] test_increaseValuesOfParticipantSinglePlayer() (gas: 313994)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.49ms (3.81ms CPU time)
​
Ran 1 test suite in 502.57ms (5.49ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended mitigation

  • Change the validation in the function ChoosingRam::increaseValuesOfParticipants to check >= as shown below and in the diff above.

    • tokenIdOfChallenger >= ramNFT.tokenCounter() and

    • tokenIdOfAnyPerticipent >= ramNFT.tokenCounter()

References

Tools Used

  • Manual Review

Updates

Lead Judging Commences

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

The token counter check is incorrect

Support

FAQs

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