Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

NFT owner can lose ownership of NFT due to revert in RapBattle

Summary

Under a specific set of circumstances, a OneShot NFT holder can lose ownership of their OneShot NFT in a rap battle and it cannot be reclaimed without protocol intervention.

Vulnerability Details

If an NFT holder enters a rap battle as the 1st entrant ("defender"), the RapBattle::goOnStageOrBattle function transfers ownership of that NFT to the RapBattle contract. A 2nd entrant ("challenger") enters the battle and matches the bet, but it is an amount of CRED which he does not possess. If the defender wins, the RapBattle::goOnStageOrBattle function will revert when it attempts to transfer CRED from the challenger to the defender. This occurs before the contract transfers the defender's NFT back to the defender, resulting in the NFT still being owned by the RapBattle contract.

Severity

Medium - Impact is Medium due to defender losing the NFT as well as the time & gas used to stake/unstake & enter battle. Likelihood is Low due to the number of factors needing to align for this attack to work.

Tools Used

Visual Studio Code, Foundry, manual review

Proof of Code

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

function testNftNotStuckWithBattleRevert() public twoSkilledRappers {
// may have to run multiple times to get owner 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);
vm.stopPrank();
// as challenger, call enter battle using their NFT id and amount of cred (which they don't have)
vm.startPrank(challenger);
cred.approve(address(rapBattle), 1 ether);
vm.expectRevert(); // should revert if defender wins because nonOwner has no CRED
rapBattle.goOnStageOrBattle(1, 1 ether);
vm.stopPrank();
// validate NFT returned to user (i.e. not stuck)
// if fails, RapBattle contract still owns w/ no way to return
assert(oneShot.ownerOf(0) == user);
}
  1. Execute the test until it fails - indicating the user is no longer the owner of the NFT.

Note: I had to hard code the value of the random variable in RapBattle::_battle to trigger a win for the defender, as in my environment the number generated kept landing on a challenger win.

Recommendations

One way to address this is to verify in RapBattle::goOnStageOrBattle that the caller has sufficient CRED to cover their bet.
This can be done by adding this line of code to the top of that function:

require(credToken.balanceOf(msg.sender) >= _credBet, "caller must possess enough CRED to cover bet");

A better way would be to simplify the protocol design by not transferring the NFT from the defender to begin with and therefore remove the need to transfer it back. This would give the defender the same treatment as the challenger (whose NFT is not transferred to RapBattle). Plus it would save gas.
To implement this change in design, simply remove these lines from the RapBattle contract:

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

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

Lead Judging Commences

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

missing check for sufficient `_credBet_` approval

Support

FAQs

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