Summary
RapBattle contract allows users to get their Rapper on the stage and compete with one another, and winner is decided on the basis of some critera.
But it doesn't increment the battlesWon of the winner, where battlesWon is a crucial part of tracking the total wins of a rapper and on the basis of which the protocol may create a leaderboard but due to not handling the wins of a rapper, the participants will not like the protocol.
The battlesWon is a part of the RapperStats struct inside OneShot contract.
Vulnerability Details
The vulnerability is present inside the RapBattle contract where the _battle function doesn't handle the incrementation of the battlesWon for the winning rapper.
battlesWon is used to track the total rap battles won by the Rapper NFT of a person which is a part of RapperStats struct in OneShot contract.
It is expected for the rapper who won the battle should have their battlesWon incremented but it doesn't have any implementation to interact with the OneShot contract and increment the winning of winner Rapper Stats.
Impact
The winnings of rapper can never be tracked.
If the protocol designs a leaderboard, as the winnings are not updated, the leaderboard will be of no use.
Tools Used
Manual Review, Unit Test in Foundry
Add the test in the file: test/OneShotTest.t.sol
Run the test:
forge test --mt test_RapBattle_WinsOfWinnerIsNotIncremented
function test_RapBattle_WinsOfWinnerIsNotIncremented() public twoSkilledRappers {
uint256 userTokenId = 0;
uint256 challengerTokenId = 1;
uint256 userInitWins = oneShot.getRapperStats(userTokenId).battlesWon;
uint256 challengerInitWins = oneShot.getRapperStats(challengerTokenId).battlesWon;
uint256 betAmount = 1;
vm.startPrank(user);
oneShot.approve(address(rapBattle), userTokenId);
cred.approve(address(rapBattle), betAmount);
rapBattle.goOnStageOrBattle(userTokenId, betAmount);
vm.stopPrank();
vm.startPrank(challenger);
cred.approve(address(rapBattle), betAmount);
rapBattle.goOnStageOrBattle(challengerTokenId, betAmount);
vm.stopPrank();
uint256 userFinalWins = oneShot.getRapperStats(userTokenId).battlesWon;
uint256 challengerFinalWins = oneShot.getRapperStats(challengerTokenId).battlesWon;
assert(userFinalWins == userInitWins);
assert(challengerFinalWins == challengerInitWins);
}
Recommendations
Increment the wins of the winner in the rapperStats mapping of OneShot contract
@@ -8,8 +8,11 @@ import {IOneShot} from "./interfaces/IOneShot.sol";
import {Streets} from "./Streets.sol";
contract OneShot is IOneShot, ERC721URIStorage, Ownable {
+ error OneShot__OnlyRapBattleCanIncrement();
+
uint256 private _nextTokenId;
Streets private _streetsContract;
+ address private rapBattleAddr;
// Mapping from token ID to its stats
mapping(uint256 => RapperStats) public rapperStats;
@@ -21,6 +24,10 @@ contract OneShot is IOneShot, ERC721URIStorage, Ownable {
_streetsContract = Streets(streetsContract);
}
+ function setRapBattleAddr(address _rapBattle) public onlyOwner {
+ rapBattleAddr = _rapBattle;
+ }
+
modifier onlyStreetContract() {
require(msg.sender == address(_streetsContract), "Not the streets contract");
_;
@@ -51,6 +58,14 @@ contract OneShot is IOneShot, ERC721URIStorage, Ownable {
metadata.battlesWon = battlesWon;
}
+ function incrementWinnings(uint256 tokenId) external {
+ if (msg.sender != rapBattleAddr) {
+ revert OneShot__OnlyRapBattleCanIncrement();
+ }
+
+ rapperStats[tokenId].battlesWon += 1;
+ }
+
/*//////////////////////////////////////////////////////////////
VIEW
//////////////////////////////////////////////////////////////*/
@@ -27,4 +27,6 @@ interface IOneShot is IERC721 {
bool calmAndReady,
uint256 battlesWon
) external;
+
+ function incrementWinnings(uint256 tokenId) external;
}
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);
+ // update the winnings of defender
+ oneShotNft.incrementWinnings(defenderTokenId);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
+ // update the winnings of challenger
+ oneShotNft.incrementWinnings(_tokenId);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
