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

battlesWon variable unused, not update after the result of goOnStageOrBattle operation when defender is not null address

0xlouistsai

Summary

The One Shot documentation does not detail the battlesWon variable within the RapperStats structure in the IOneShot interface. Given the variable's name, we infer that it is meant to represent the number of battles won by the rapper. However, the battlesWon variable remains unaltered throughout the entire codebase, rendering it unused.

Vulnerability Details

Assuming two rappers, Bob and Alice, engage in a battle, the battlesWon variable for one of them should increase from zero to a specific number, as outlined in the white paper. To verify this behavior, the testBattlesWonIncreaseOrNot test can be added to RapBattleTest. Upon running the command forge test --match-test testBattlesWonIncreaseOrNot, the test fails, indicating that the battlesWon variables are not updating as expected.

function testBattlesWonIncreaseOrNot() public {
address Bob = makeAddr("Bob");
IOneShot.RapperStats memory bobStats;
vm.prank(Bob);
oneShot.mintRapper(); // tokenId: 0
​
address Alice = makeAddr("Alice");
IOneShot.RapperStats memory aliceStats;
vm.prank(Alice);
oneShot.mintRapper(); // tokenId: 1
​
vm.startPrank(Bob);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 0); // Bob will be the defender
vm.stopPrank();
​
vm.startPrank(Alice);
oneShot.approve(address(rapBattle), 1);
rapBattle.goOnStageOrBattle(1, 0); // Alice will battle with Bob
vm.stopPrank();
​
bobStats = oneShot.getRapperStats(0);
aliceStats = oneShot.getRapperStats(1);
// We do not know the result of the battle, however, the battlesWon variable should increase in rapper status of Bob or Alice
assertTrue(bobStats.battlesWon==1 || aliceStats.battlesWon==1);
}

Impact

The battlesWon variable, although present in the code, remains unused. This lack of utilization may lead to confusion for users seeking clarity on its intended purpose and meaning within the context of the codebase.

Tools Used

Manual Review

Recommendations

Either remove the battlesWon variable or ensure its update within the _battle function.

function _battle(uint256 _tokenId, uint256 _credBet) internal {
...
​
defender = address(0);
emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
+ uint256 tokenId = random < defenderRapperSkill? defenderTokenId: _tokenId;
// If random <= defenderRapperSkill -> defenderRapperSkill wins, otherwise they lose
if (random <= defenderRapperSkill) {
// We give them the money the defender deposited, and the challenger's bet
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
+ IOneShot.RapperStats memory rapperStats = oneShotNft.getRapperStats(tokenId);
+ rapperStats.battlesWon += 1;
+ oneShotNft.updateRapperStats(
+ tokenId,
+ rapperStats.weakKnees,
+ rapperStats.heavyArms,
+ rapperStats.spaghettiSweater,
+ rapperStats.calmAndReady,
+ rapperStats.battlesWon
+ );
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId); // @audit-issue not update battlesWon
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`battlesWon` is never updated

Support

FAQs

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