Summary
An attacker can cheat RapBattle::_battle by not approving RapBattle contract address to spend their CredToken if they loose bet
Vulnerability Details
An attacker can check if a defender exists therefore, decide not to approve RapBattle contract to spend their CredToken because the else block gets called in goOnStageOrBattle thereby not transferring any tokens and code execution moves to _battle function.
Within the _battle function, RapBattle transfers only the defender CredToken to a winning challenger and for the case where a challenger looses, within the if block, the RapBattle contract tries to transfer challenger's CredToken to defender but it gets reverted hence, challenger knows they lost bet but retained their CredToken
Impact
An attacker leverages the misplaced code logic to ensure they never loose bet even when their rapper skills is below that of defender
Proof of Concept: Proof of Code
Within your foundry test suite, include the code below ⬇️:
Code
function testChallengerCanChooseNotApproveRapBattleToBet() public {
(uint256 defenderTokenId, uint256 challengerTokenId) = readyPlayersForBattle();
uint256 _credBet = 3;
vm.startPrank(defender);
oneShotTokenContract.approve(address(rapBattleContract), defenderTokenId);
credTokenContract.approve(address(rapBattleContract), _credBet);
rapBattleContract.goOnStageOrBattle(defenderTokenId, _credBet);
vm.stopPrank();
vm.prank(challenger);
vm.expectRevert();
rapBattleContract.goOnStageOrBattle(challengerTokenId, _credBet);
}
On your terminal, run the command below:
forge test --mt testChallengerCanChooseNotApproveRapBattleToBet -vvvvv
RESULT
Running 1 test for test/FindingsTest.t.sol:FindingsTest
[PASS] testChallengerCanChooseNotApproveRapBattleToBet() (gas: 584792)
...
├─ [28309] RapBattle::goOnStageOrBattle(1, 3)
│ ├─ [1183] OneShot::getRapperStats(0) [staticcall]
│ │ └─ ← RapperStats({ weakKnees: false, heavyArms: false, spaghettiSweater: false, calmAndReady: false, battlesWon: 0 })
│ ├─ [1183] OneShot::getRapperStats(1) [staticcall]
│ │ └─ ← RapperStats({ weakKnees: false, heavyArms: false, spaghettiSweater: false, calmAndReady: false, battlesWon: 0 })
│ ├─ emit Battle(challenger: Challenger: [0x846F7fB58d70E74E7663287da63f88E9F8dD8fdf], tokenId: 1, winner: Challenger: [0x846F7fB58d70E74E7663287da63f88E9F8dD8fdf])
│ ├─ [18516] Credibility::transfer(Defender: [0x7E6A50Ec13a3762C32fffEb425B71Bf40668dC46], 3)
│ │ ├─ emit Transfer(from: RapBattle: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], to: Defender: [0x7E6A50Ec13a3762C32fffEb425B71Bf40668dC46], value: 3)
│ │ └─ ← true
│ ├─ [2926] Credibility::transferFrom(Challenger: [0x846F7fB58d70E74E7663287da63f88E9F8dD8fdf], Defender: [0x7E6A50Ec13a3762C32fffEb425B71Bf40668dC46], 3)
│ │ └─ ← ERC20InsufficientAllowance(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0, 3)
│ └─ ← ERC20InsufficientAllowance(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0, 3)
└─ ← ()
Tools Used
Recommendations
Refactor goOnStageOrBattle function as follows:
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);
}
}
Refactor RapBattle::_battle function as follows:
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, (defenderBet + _credBet));
} else {
- // Otherwise, since the challenger never sent us the money, we just give the money in the contract
+ // we give them the money the defender deposited, and the challenger's bet
- credToken.transfer(msg.sender, _credBet);
+ credToken.transfer(msg.sender, (defenderBet + _credBet));
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
