Summary
A user can pass any _tokenId into RapBattle::goOnStageOrBattle() without actually owning that NFT to challenge a defender.
Vulnerability Details
the RapBattle::goOnStageOrBattle() function is meant to assign a defender if there is no defender, else allow a challenger to challenge the defender. It requires 2 arguments, a uint256 representing a Rapper's NFT ID and a uint256 representing how many cred tokens to bet:
function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
if (defender == address(0)) {
defender = msg.sender;
defenderBet = _credBet;
defenderTokenId = _tokenId;
emit OnStage(msg.sender, _tokenId, _credBet);
oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
credToken.transferFrom(msg.sender, address(this), _credBet);
} else {
_battle(_tokenId, _credBet);
}
}
If defender is address 0, then we set the defender variable to msg.sender, set the cred bet, and the rapper ID that will be defending. It then emits an event, and transfers the rapper NFT in addition to the tokens to the contract.
If the defender address is not address 0, then we have a defender, and the caller msg.sender of RapBattle::goOnStageOrBattle() will be the challenger. Note that execution in the else block goes into RapBattle::_battle() passing in the _tokenId for the first argument and does not check to see if msg.sender owns the _tokenId that is passed in. If we look into RapBattle::_battle() function:
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;
defender = address(0);
emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
if (random <= defenderRapperSkill) {
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
We'll see that _tokenId is only referenced twice. First for getting the rapper's skill by calling RapBattle::getRapperSkill(), the 2nd for emitting the Battle event. If we look at RapBattle::getRapperSkill():
function getRapperSkill(
uint256 _tokenId
) public view returns (uint256 finalSkill) {
IOneShot.RapperStats memory stats = oneShotNft.getRapperStats(_tokenId);
finalSkill = BASE_SKILL;
if (stats.weakKnees) {
finalSkill -= VICE_DECREMENT;
}
if (stats.heavyArms) {
finalSkill -= VICE_DECREMENT;
}
if (stats.spaghettiSweater) {
finalSkill -= VICE_DECREMENT;
}
if (stats.calmAndReady) {
finalSkill += VIRTUE_INCREMENT;
}
}
This function then calls OneShot::getRapperStats() passing in the _tokenId to get the stats of the token:
function getRapperStats(uint256 tokenId) public view returns (RapperStats memory) {
return rapperStats[tokenId];
}
Since throughout the remainder of the RapBattle::_battle() function there is no other reference to the _tokenId to ensure the challenger msg.sender owns the rapper, this allows any user to challenge a defender with any _tokenId that they don't own.
Impact
POC
Here is how the scenario can play out:
User A calls OneShot::mintRapper() and gets NFT #0.
User A calls RapBattle::goOnStageOrBattle() to place a bet
User B calls OneShot::mintRapper() and gets NFT #1.
User C calls RapBattle::goOnStageOrBattle() passing in _tokenId of NFT #1.
User C wins the bet and gets rewarded the defender tokens.
Below you can see a POC of the above scenario:
function testUserNotApprovingNFTBeforeBattle() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 0);
vm.stopPrank();
address attacker = makeAddr("attacker");
vm.prank(attacker);
vm.warp(2 hours);
vm.recordLogs();
rapBattle.goOnStageOrBattle(1, 0);
vm.stopPrank();
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
assert(winner == attacker);
}
Tools Used
VS Code, Foundry
Recommendations
Consider adding a check to ensure the challenger owns their NFT. Note that this would require their NFT to not be staked in the Streets contract:
function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
+ require(
+ oneShotNft.ownerOf(_tokenId) == msg.sender,
+ "RapBattle: Not the owner"
+ );
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);
} 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;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
