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

`RapBattle::goOnStageOrBattle()` allows anyone to use an NFT they do not own to become the challenger

0xlrivo

Summary

RapBattle:goOnStageOrBattle() allows anyone to use an NFT they do not own to become the challenger.

Vulnerability Details

The vulnerability is caused by the fact that both RapBattle::goOnStageOrBattle() and RapBattle::_battle() do not require the challenger to prove he's actually the owner of the NFT with tokenId passed as a parameter.

This way, anyone can use an NFT they do not own (maybe with an higher stats compared to their) to become the challenger.

Impact

Anyone can falsely use another user's NFT to become the challenger.

Tools Used

  • Manual Review

Add the following test to demonstrate the issue:

​
function _mintAndStakeRapper(address _user, uint256 _stakingDays) public returns(uint256 mintedTokenId) {
vm.startPrank(_user);
mintedTokenId = oneShot.getNextTokenId();
oneShot.mintRapper();
assert(oneShot.ownerOf(mintedTokenId) == _user);
oneShot.approve(address(streets), mintedTokenId);
streets.stake(mintedTokenId);
vm.warp(block.timestamp + (_stakingDays * 1 days) + 1 seconds);
streets.unstake(mintedTokenId);
vm.stopPrank();
}
​
function test_challengerCanBeNonOwner() public {
address user1 = makeAddr("user1");
uint256 defenderNft = _mintAndStakeRapper(user, 2);
uint256 user1Nft = _mintAndStakeRapper(user1, 2);
uint256 challengerNft = _mintAndStakeRapper(challenger, 2);
​
vm.startPrank(user);
oneShot.approve(address(rapBattle), defenderNft);
cred.approve(address(rapBattle), 2);
rapBattle.goOnStageOrBattle(defenderNft, 2);
vm.stopPrank();
​
// challenger do not own user1Nft
assert(oneShot.ownerOf(user1Nft) != challenger);
​
vm.prank(challenger);
rapBattle.goOnStageOrBattle(user1Nft, 2);
}

Recommendations

Force the challenger to transfer his NFT so you can be sure that:

  • the NFT actually exists

  • the challenger actually owns the NFT

Alternatively, if you don't want to waste gas for the two additional transfers required for this first fix, you can just add the following checks inside goOnStageOrBattle:

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
if (defender == address(0)) {
// ...
} else {
+ require(oneShotNft.ownerOf(_tokenId) == msg.sender);
+ require(credToken.balanceOf(msg.sender) >= _credBet);
_battle(_tokenId, _credBet);
}
}

Note: if the _tokenId do not exists ERC721::ownerOf() will revert with ERC721NonexistentToken

Note: we can also assert before entering the battle that the challenger has enough cred tokens to pay if he loses. It isn't mandatory since the transaction will revert later even without it but, by checking before entering _battle(), we can save some gas.

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.