First Flight #10: One Shot
First Flight #10
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Incorrect implementation to decide the winner inside `RapBattle::_battle` function

shikhar229169

Summary

The RapBattle contract allows users to get their rapper NFT on stage for battle and winner is decided on the basis of rapper's skill which is used to weight their likelihood of randomly winning the battle.

Here, randomness is used to get a random number on the basis of which the winner is decided based on their skills.
But, the implementation to decide the winner for a particular scenario when the random variable is equal to defenderRapperSkill is wrong, the winner is set to defender instead of challenger.

Vulnerability Details

The vulnerability is present in the RapBattle::_battle function where the winner deciding mechanism is incorrectly implemented.
The protocol calculate the winner as below:

It uses the sum of skills of both the rappers to calculate the random number by taking a modulo with the sum, therefore the random number lies in the range from 0 to totalBattleSkill - 1, where totalBattleSkill being the sum of skills of both the competing rappers.

And finally, to decide the winner if the random number is less than or equal to the defenderRapperSkill, defender is considered the winner otherwise challenger is considered the winner.

But there is a catch over here, the range of values for which it considers defender the winner is from 0 to defenderRapperSkill but challenger is considered a winner for the values from defenderRapperSkill + 1 to totalBattleSkill - 1.

  • For defender: 0 to defenderRapperSkill which is a span of defenderRapperSkill - 0 + 1 = defenderRapperSkill + 1

  • For challenger: defenderRapperSkill + 1 to totalBattleSkill - 1 which is a span of totalBattleSkill - 1 - defenderRapperSkill - 1 + 1 = totalBattleSkill - defenderRapperSkill - 1 = challengerRapperSkill - 1.

We can clearly see that defender is considered a winner for the span of defenderRapperSkill + 1 but for challenger it is challengerRapperSkill - 1, thus giving advantage to the defender by considering one extra skill span for their winning.
The correct implementation should be to consider the span for defender as defenderRapperSkill and challenger as challengerRapperSkill.

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;
​
@> // 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);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
}

Impact

For the case when random number is equal to defenderRapperSkill, defender is considered the winner instead of challenger.

Tools Used

Manual Review

Recommendations

Make the necessary changes to consider challenger as winner when random number is >= defenderRapperSkill.

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 -> defenderRapperSkill wins, otherwise they lose
- if (random <= defenderRapperSkill) {
+ 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);
} else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Defender's advantage

shikhar229169 Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Defender's advantage

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.