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

`RapBattle.goOnStageOrBattle` allows submit `tokenId` to a Battle without owning it.

Ritos

Vulnerability Details

Function goOnStageOrBattle in contract RapBattle if defender != address(0) do not check that msg.sender is owner of rapper with _tokenId.

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);
}
}

POC

Assume that defender in the RapBattle contract equals address(0), which means that there is no one on stage right now.

1 First user call RapBattle.goOnStageOrBattle with his own rapper.

2 Attacker call RapBattle.goOnStageOrBattle with some rapper with RapBattle.getRapperSkill equal 75.

3 Function _battle in the RapBattle contract then do incorrect computations of winner:

3.1 challengerRapperSkill manipulated by attacker up to 25 points

3.2 totalBattleSkill depends on challengerRapperSkill :

uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;

3.3 random is in [0, totalBattleSkill) :

uint256 random = uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;

3.4 chance to win depends on totalBattleSkill :

if (random <= defenderRapperSkill)

assume defenderRapperSkill equal 50 this means:

  • in the situation where attacker uses most skilled rapper without owning him with challengerRapperSkill equal 75:

defender win in 50 from 125 randoms => 40% chance to win

  • in the situation where attacker uses just minted rapper with challengerRapperSkill equal 50:

defender win in 50 from 100 randoms => 50% chance to win

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);
} 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);
}

Impact

It allows any user to take part in a battle without owning a rapper and choose the rapper with the biggest skill returned from RapBattle.getRapperSkill.
This leads to improve his chances to win up to 10%.

Tools Used

Manual review.

Recommendations

Make the following changes in RapBattle.goOnStageOrBattle

https://github.com/Cyfrin/2024-02-one-shot/blob/47f820dfe0ffde32f5c713bbe112ab6566435bf7/src/RapBattle.sol#L38C1-L52C6

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
+ require(oneShotNft.ownerOf(_tokenId) == msg.sender);
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);
}
}
Updates

Lead Judging Commences

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

Challenger can use any nft to battle - not necessarily theirs

Support

FAQs

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