First Flight #10: One Shot

Summary

In the `RapBattle::_battle` function, the winner of the battle receive only one
part of the total money to earn.

Vulnerability Details

```
    diff
    function _battle(uint256 _tokenId, uint256 _credBet) internal {
        address _defender = defender;
        require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
        uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
        uint256 challengerRapperSkill = getRapperSkill(_tokenId);
        uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
        uint256 totalPrize = defenderBet + _credBet;

        uint256 random =
            uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;

        // Reset the defender
        defender = address(0);
        emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);

        // If random <= defenderRapperSkill -> defenderRapperSkill wins, otherwise they lose
        if (random <= defenderRapperSkill) {
            // We give them the money the defender deposited, and the challenger's bet
@=>     credToken.transfer(_defender, defenderBet);
@=>     credToken.transferFrom(msg.sender, _defender, _credBet);
        } else {
            // Otherwise, since the challenger never sent us the money, we just give the money in the contract
@=>     credToken.transfer(msg.sender, _credBet);
        }
        totalPrize = 0;
        // Return the defender's NFT
        oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
    }
```

In the `RapBattle` contract, the money deposited by the defender is sent to this contract 
and stored in state variable `RapBattle::defenderBet`. 

On the other hand, the money put into play by the challenger is not sent to this contract, 
but remains in parameter `_credBet` of the _battle function.

So if it's the defender who wins, we must transfer the challenger's stake money to him, 
which is stored in parameter `_credBet`, and his money, which is stored in state variable `defenderBet`.

And if the challenger wins, we just have to transfer the defender's money, 
which has been stored in the `defenderBet` variable of the `RapBattle` contract.

Impact

The money wagered by the two players is not paid out in full to the winner as 
it should be; part of it remains locked in the contract.

For example, this test, run 10 days after a stake, passes without error.
```
    // Test rapper stats are updated when a rapper is staked for at least one day
    function testRapperStatsUpdatedWhenRapperStakedForOneDay() public mintRapper {
        vm.startPrank(user);
        oneShot.approve(address(streets), 0);
        streets.stake(0);
        vm.stopPrank();
    @=> vm.warp(10 days + 1);
        vm.startPrank(user);
        streets.unstake(0);

        stats = oneShot.getRapperStats(0);
        assert(stats.weakKnees == false);
        assert(stats.heavyArms == false);
        assert(stats.spaghettiSweater == false);
        assert(stats.calmAndReady == true);
        assert(stats.battlesWon == 0);
    }
```   

Tools Used

-Foundry
-fuzz test

Recommendations

Modify the `RapBattle::_battle` function as follows:

```diff
    function _battle(uint256 _tokenId, uint256 _credBet) internal {
        address _defender = defender;
        require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
        uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
        uint256 challengerRapperSkill = getRapperSkill(_tokenId);
        uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
        uint256 totalPrize = defenderBet + _credBet;

        uint256 random =
            uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;

        // Reset the defender
        defender = address(0);
        emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);

        // If random <= defenderRapperSkill -> defenderRapperSkill wins, otherwise they lose
        if (random <= defenderRapperSkill) {
            // We give them the money the defender deposited, and the challenger's bet
        -    //credToken.transfer(_defender, defenderBet);
        +    credToken.transfer(_defender, _credBet);
        -    //credToken.transferFrom(msg.sender, _defender, _credBet);
        +    credToken.transferFrom(msg.sender, _defender, defenderBet);
        } else {
            // Otherwise, since the challenger never sent us the money, we just give the money in the contract
        -    //credToken.transfer(msg.sender, _credBet);
        +    credToken.transferFrom(msg.sender, ownerOf(_tokenId), defenderBet);
        }
        totalPrize = 0;
        // Return the defender's NFT
        oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
    }
```