First Flight #10: One Shot

Summary

In contract OneShot function mintRapper() due to not following pattern Checks Effects Interactions lead to cross contract reentrancy.

Vulnerability Details

In contract OneShot function mintRapper() do not following Checks Effects Interactions pattern.

Function mintRapper() call _safeMint which trigger onERC721Received function call on msg.sender if its contract.

Only after that function mintRapper() change the state for new rapper:

POC

1 First User call OneShot.mintRapper()

2 First User call RapBattle.goOnStageOrBattle()

3 Attacker monitoring mempool for the goOnStageOrBattle() transaction of first user.

4 Attacker put his transaction right after first user goOnStageOrBattle() transaction.

5 In this transaction attacker trigger his contract which is IERC721Receiver to do following steps:

5.1. call OneShot.mintRapper()

5.2. in function onERC721Received which trigger by the OneShot contract call RapBattle.goOnStageOrBattle()

6 Contract RapBattle calls _battle() inside goOnStageOrBattle()

7 Function _battle() trigger following calls RapBattle.getRapperSkill -> OneShot.getRapperStats

8 At this stage contract OneShot already minted an attacker rapper, but not properly Initialize metadata for attacker rapper.

9 Function OneShot.getRapperStats returns wrong stats for attacker rapper

10 Function RapBattle.getRapperSkill returns 65 point for the attacker rapper, instead of 50

11 Function _battle() performs incorrect winner calculations:

The RapBattle._battle() to calculate a winner use totalBattleSkill which depends on challengerRapperSkill:

To determinate winner RapBattle._battle() compares random with defenderRapperSkill.

As we can see the random belongs to set of integers from 0 to totalBattleSkill.

Attacker can manipulate the totalBattleSkill adding to his skill 15 points.

First User rapper has defenderRapperSkill = 50

Attacker rapper has challengerRapperSkill = 65, although the correct value without reentrancy would be 50

Without reentrancy defender wins in 50 randoms from 100 // 50% of winning

With reentrancy defender wins in 50 randoms from 115 // ~43% of winning

Impact

Attacker can increase his chances to win up to 7% without staking an Rapper NFT in Streets.

Tools Used

Manual review.

Recommendations

Make the following changes in OneShot.mintRapper()

https://github.com/Cyfrin/2024-02-one-shot/blob/47f820dfe0ffde32f5c713bbe112ab6566435bf7/src/OneShot.sol#L29C1-L36C6