Summary
battlesWon is an element of the OneShot::RapperStats structure, a piece of the NFT's metadata, and is supposed to track how many battles has been won with a given rapper NFT. However, this element is never updated in the protocol, it remains 0 even if battles are won with an NFT.
Impact
The metadata of the NFTs will be incorrect. Specifically, the battlesWon statistics will not correctly reflect the number of battles an NFT has previously won.
Proof of Code
Insert the following piece of code in OneShotTest.t.sol:
function test_battlesWonNotUpdated() public twoSkilledRappers {
vm.startPrank(user);
uint256 initialuserBalance = cred.balanceOf(user);
cred.approve(address(rapBattle), initialuserBalance);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, initialuserBalance);
vm.stopPrank();
vm.startPrank(challenger);
cred.approve(address(rapBattle), initialuserBalance);
vm.recordLogs();
rapBattle.goOnStageOrBattle(1, initialuserBalance);
vm.stopPrank();
Vm.Log[] memory entries_2 = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries_2[0].topics[2])));
assertEq(winner, challenger);
assertEq(oneShot.ownerOf(1), challenger);
stats = oneShot.getRapperStats(1);
assertEq(stats.battlesWon, 0);
}
and run it by executing forge test --mt test_battlesWonNotUpdated.
Output:
Running 1 test for test/OneShotTest.t.sol:RapBattleTest
[PASS] test_battlesWonNotUpdated() (gas: 640173)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.50ms
Tools Used
Manual reivew, Foundry.
Recommendations
There are 2 possible solutions:
remove this battlesWon element from the OneShot::RapperStats structure, and update the rest of the codebase accordingly. (Especially since it seems to be only a vanity metric that has no effect anywhere.), or
update the battlesWon element after every battle as follows:
function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
uint256 challengerRapperSkill = getRapperSkill(_tokenId);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
uint256 totalPrize = defenderBet + _credBet;
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;
// Reset the defender
defender = address(0);
emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
// 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);
+ IOneShot.RapperStats memory stats = oneShotNft.getRapperStats(defenderTokenId);
+ uint256 updatedBattlesWon = stats.battlesWon + 1;
+ oneShotNft.updateRapperStats(defenderTokenId, stats.weakKnees, stats.heavyArms, stats.spaghettiSweater, stats.calmAndReady, updatedBattlesWon);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
+ IOneShot.RapperStats memory stats = oneShotNft.getRapperStats(_tokenId);
+ uint256 updatedBattlesWon = stats.battlesWon + 1;
+ oneShotNft.updateRapperStats(_tokenId, stats.weakKnees, stats.heavyArms, stats.spaghettiSweater, stats.calmAndReady, updatedBattlesWon);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
