First Flight #10: One Shot
First Flight #10
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

`OneShot.getRapperStats()` returns incorrect stats for not minted `tokenId`s

Ritos

Vulnerability Details

Function getRapperStats in OneShot contract not checked that tokenId is belong to previously minted NFT which leads to the return of incorrect data for non-existent NFTs:

weakKnees, heavyArms, spaghettiSweater is set to false that leads to incorrect RapBattle.getRapperSkill(uint256 _tokenId) returns:

1 RapBattle.getRapperSkill(uint256 _tokenId) returns 50 for just minted rapper.
2 RapBattle.getRapperSkill(uint256 _tokenId) returns 65 for non-existent rapper.

Assume that defender getRapperSkill equal 50 then:

Without using this vulnerability by attacker defender wins in 50 randoms from 100 // 50% of winning

With using this vulnerability by attacker defender wins in 50 randoms from 115 // ~43% of winning

Impact

Incorrect data leads to confusion users of the system.

Also attacker can use this in conjunction with another vulnerability (RapBattle.goOnStageOrBattle does not check the ownership of the _tokenId supplied by challenger) and increase his chances to win up to 7% without minting and staking rapper.

POC

Add this test to OneShotTest.t.sol and run via forge test --mt testGetRapperStatsForNotExistingRapper to see it success.

function testGetRapperStatsForNotExistingRapper() public {
address testUser = makeAddr("Bob");
vm.startPrank(testUser);
uint notExistingRapper = oneShot.getNextTokenId();
IOneShot.RapperStats memory rpStats = oneShot.getRapperStats(notExistingRapper);
​
assert(rpStats.weakKnees == false);
assert(rpStats.heavyArms == false);
assert(rpStats.spaghettiSweater == false);
assert(rpStats.calmAndReady == false);
assert(rpStats.battlesWon == 0);
}

Output:

Running 1 test for test/OneShotTest.t.sol:RapBattleTest
[PASS] testGetRapperStatsForNotExistingRapper() (gas: 18360)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.36ms

Tools Used

Manual review, foundry.

Recommendations

Make the following changes in OneShot.getRapperStats()

https://github.com/Cyfrin/2024-02-one-shot/blob/47f820dfe0ffde32f5c713bbe112ab6566435bf7/src/OneShot.sol#L57C1-L59C6

function getRapperStats(uint256 tokenId) public view returns (RapperStats memory) {
+ require(tokenId < _nextTokenId, "Wrong tokenId");
return rapperStats[tokenId];
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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