Summary
At the end of a battle, the battlesWon variable is not being updated for the winning rapper.
Vulnerability Details
When determining the winner of a battle, the battlesWon variable of the winning rapper should be updated. Otherwise, there will be no record of how many battles a rapper has won.
In this test, we verify that the winner does not see their battlesWon counter updated.
function testBattlesWonNotUpdated(uint256 blockNumber) public twoSkilledRappers {
vm.assume(blockNumber < type(uint256).max / 12);
vm.roll(blockNumber);
vm.warp(blockNumber * 12);
vm.prevrandao(keccak256(abi.encodePacked(block.number)));
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
vm.startPrank(challenger);
oneShot.approve(address(rapBattle), 1);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
OneShot.RapperStats memory rapperStats0 = oneShot.getRapperStats(0);
OneShot.RapperStats memory rapperStats1 = oneShot.getRapperStats(1);
assert(rapperStats0.battlesWon == 0 && rapperStats1.battlesWon == 0);
}
Test passes
Ran 1 test for test/OneShotTestAudit.t.sol:RapBattleTest
[PASS] testBattlesWonNotUpdated(uint256) (runs: 256, μ: 637406, ~: 635544)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 152.55ms
Impact
Rappers will not be able to see their updated battlesWon counter after winning a battle, which can affect dapps or other protocols.
Tools Used
Foundry, Manual review
Recommendations
Add a method to OneShot.sol to update battlesWon of the winner after a battle. This method needs a modifier and a setter similar to _streetsContract:
+address private _battleContract;
+function setBattleContract(address battleContract) public onlyOwner {
+ _battleContract = battleContract;
+}
+modifier onlyBattleContract() {
+ require(msg.sender == _battleContract, "Not the battle contract");
+ _;
+}
+function incrementBattlesWon(uint256 tokenId) external onlyBattleContract {
+ rapperStats[tokenId].battlesWon++;
+}
Update the interface in IOneShot.sol
+function incrementBattlesWon(uint256 tokenId) external;
Call the new method to update the battlesWon variable of the winning rapper in _battle()
// 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);
+ oneShotNft.incrementBattlesWon(defenderTokenId);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
+ oneShotNft.incrementBattlesWon(_tokenId);
}
totalPrize = 0;
}```
To test, modify the assertion in the previous test:
```diff
- assert(rapperStats0.battlesWon == 0 && rapperStats1.battlesWon == 0);
+ assert(rapperStats0.battlesWon == 1 || rapperStats1.battlesWon == 1);
