Summary
A user can call the RapBattle::goOnStageOrBattle() function to challenge a defender without needing to approve their tokens.
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:
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);
}
}
When a defender has been assigned, and another user calls this function to challenge the defender, the internal _battle() function is called. In that function we determine the winner. If the winner is the defender then tokens are meant to be transferred from msg.sender (the challenger) to the defender via the CredToken::transferFrom() 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);
}
The CredToken contract is a ERC20 contract therefore the caller of transferFrom() must be approved to transfer tokens on behalf of the user. If the msg.sender (the challenger) of RapBattle::goOnStageOrBattle() doesn't approve and the defender wins, this results in the function reverting.
Whereas if msg.sender (the challenger) of RapBattle::goOnStageOrBattle() doesn't approve and they win, this results in them earning tokens regardless of if they approved or not.
It's worth noting that in the scenario that msg.sender (the challenger) does approve the contract to transfer their tokens, and they win, this results in CredToken::transferFrom() not being executed and leaving the challengers CredToken::_allowances to not be updated.
Impact
Allows a challenger to only successfully call RapBattle::goOnStageOrBattle() when they win, thus not risking any of their tokens.
Prevents a defender from earning tokens from a challenge they should've won.
Causes CredToken::_allowances to not be updated when a challenger approves and wins.
POC
Below you can see how a scenario plays out where the defender is expected to win but the challenger doesn't approve their CredTokens:
function testRapperNotApprovingBeforeBattle() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 10);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
vm.startPrank(challenger);
oneShot.approve(address(rapBattle), 1);
vm.warp(1 days);
vm.expectRevert();
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
}
Tools Used
VS Code, Foundry
Recommendations
Call CredToken::transferFrom() before calling CredToken::_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 {
- // credToken.transferFrom(msg.sender, address(this), _credBet);
+ credToken.transferFrom(msg.sender, address(this), _credBet);
_battle(_tokenId, _credBet);
}
}
Then update CredToken::_battle_() to transfer the totalPrize of tokens since both the defender and challenger will have already transferred their tokens:
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, totalPrize);
} else {
- // Otherwise, since the challenger never sent us the money, we just give the money in the contract
- credToken.transfer(msg.sender, _credBet);
+ // Otherwise, send the prize to the challenger
+ credToken.transfer(msg.sender, totalPrize);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
