Summary
The cred tokens and the NFT of defender can be locked in RapBattle contract forever. The vulnerability arises in RapBattle::_battle function, where the contract tries to transfer tokens from the challenger to the defender in case of lost rap battle, without verifying that the challenger has approved his tokens to be transfered. Based on the protocol's design, first the defender transfers his tokens and NFT:
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);
}
}
A griefer can simply not approve his bet to the contract since he doesn't transfer them to the protocol in anyway, or even worse he will be incentivized to do it, because if he loses his opponent will not receive his tokens, and in case of win this will not impact the transfer of his reward.
There is also second scenario, due to the predictable randomness of the global variables used in the winner-deciding random variable in the _battle function. If a user runs a strong node he can easily calculate the outcome result and if it's not in his favor, he will not approve the transfer.
@> uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;
Vulnerability Details
Add the following in OneShotTest.t.sol file:
function testChallengerFailsTransfer() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
assert(oneShot.ownerOf(0) == address(rapBattle));
assert(cred.balanceOf(address(rapBattle)) == 3);
assert(cred.balanceOf(user) == 1);
vm.startPrank(challenger);
vm.roll(block.number + 1);
vm.warp(block.timestamp + 1);
vm.expectRevert();
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
assert(oneShot.ownerOf(0) == address(rapBattle));
assert(cred.balanceOf(address(rapBattle)) == 3);
}
Impact
In case of unauthorized token transfer, the transaction will revert leaving the defender's cred tokens and NFT locked in the protocol, since there is no redeem function.
Tools Used
Foundry
Recommendations
There are few recommendations:
Implement a withdraw function, so in such cases a user can take his tokens back.
Make sure that a challenger approves his tokens to be transfered by the contract.
Enforce the second participant to transfer his tokens to the contract like the first one. This will require some changes:
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);
_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);
@> // No need for two tx process
+ 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);
+ credToken.transfer(msg.sender, totalPrize);
}
@> // This is redundant, since it's a memory variable in anyway
- totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
