First Flight #10: One Shot

Summary

The RapBattle::BASE_SKILL is assigned a value of 65. However, according to the documentation, a base skill of 50 is supposed to be applied to all rappers.

documentation says: A base skill of 50 is applied to all rappers in battle, and this is modified by the properties the rapper holds. However see the actual implementation.

• RapBattle.sol

Another flaw is the incorrect calculation of a rapper's skill in the RapBattle::getRapperSkill function. This issue is further highlighted in the documentation, where it is specified that there exists a VIRTUE INCREMENT | VICE DECREMENT for each rapper. See below:

• WeakKnees - False = +5

• HeavyArms - False = +5

• SpaghettiSweater - False = +5

• CalmAndReady - True = +10

Each rapper's skill is then used to weight their likelihood of randomly winning the battle!

The presence of a VICE DECREMENT for each rapper is also specified in the natspec documentation. However the natspec is correct but not GAS efficient.

• RapBattle.sol

It appears that only the VICE DECREMENT is being accounted but in a wrong way (Not GAS Efficient) although the calculation is right, for in the RapBattle::getRapperSkill function.

• RapBattle::getRapperSkill

As i said the calculation is right but not GAS efficient. Implementation follows the bad logic to calculate finalSkill which indeed lead to a extra unwanted GAS usage each time whenever users put their cred online and battle against each other.

Vulnerability Details

Let's analyze a rap battle scenario with a bet of 1 cred, considering the worst-case scenario with the current implementation:

Worst-case scenario:

• Defender with:

  • weakKnees: false

  • heavyArms: true

  • spaghettiSweater: true

  • calmAndReady: false

• Challenger with:

  • weakKnees: false

  • heavyArms: false

  • spaghettiSweater: false

  • calmAndReady: true

• Considering 65 as the BASE_SKILL:

  • Defender's skill = 65 - 5 - 5 = 55

  • Challenger's skill = 65 + 10 = 75

  • Defender: 55 / 130 = 42.31% chance to win

  • Challenger: 75 / 130 = 57.69% chance to win

• Considering 50 as the BASE_SKILL:

  • Defender's skill = 50 - 5 - 5 = 40

  • Challenger's skill = 50 + 10 = 60

  • Defender: 40 / 100 = 40% chance to win

  • Challenger: 60 / 100 = 60% chance to win

• Change in Winning Chance:

  • Defender's chances to win: 42.31 - 40 = +2.31% ↗️

  • Challenger's chances to win: 60 - 57.69 = -2.31% ↙️

Although, All the calculation is incorrect because implementer took BASE_SKILL == 65 and also VICE DECREMENT wrongly irrespective to docs which isn't GAS efficient.

With regards to GAS inefficiency, the issue arises when calculating stats, as the implemented logic consumes more GAS. This occurs because the logic deducts 5 points each time it encounters a VICE in the rapper, which is problematic because a rapper's VICES are completely eliminated when its user reaches full maturity with 4 CRED Tokens. An alternative efficient approach would be to increase the finalSkill points by +5 whenever there's an increment in the user's experience (cred ERC20).

• RapBattle::getRapperSkill

Impact

Even though the require(defenderBet == _credBet, "RapBattle: Bet amounts do not match") check is present in the _battle function, the BASE_SKILL can still influence the winning chances. Consequently, a user could bet for CRED <= 4, allowing them a maximum of four opportunities to battle with users who have 1 CRED, 2 CRED, 3 CRED, and 4 CRED.

If we adhere to the current implementation with a BASE_SKILL of 65 for RapBattle, the following outcomes would occur:

In the worst-case scenario, if the challenger embodies more virtue while the defender is full of vice, there is a decrement of -2.31% in the challenger's winning chances and an increment of +2.31% in the defender's winning chances, and vice versa. However, in the best-case scenario, there is no change, maintaining a constant 50%-50% chance.

• Likelihood:

  • This situation occurs frequently whenever a Rap Battle is conducted online.

  • Therefore, it should be categorized as High likelihood.

• Severity:

  • It directly influences the winning probabilities of both parties involved, namely the challenger and the defender.

  • The impact extends to affect the associated betting rewards as well.

  • Indeed, the probability distribution cannot result in a 1:99 ratio, as users must possess at least 1 CRED, which necessitates staking their ERC721 NFT to earn CRED. Therefore, such extreme probabilities are not feasible within the context of the protocol's requirements.

  • Although, It moderately impacts RapBattle's functionality.

  • The RapBattle::getRapperSkill also consumes more GAS.

  • Thus, severity should be Medium.

Considering both likelihood and severity, the overall impact is deemed MEDIUM.

Tools Used

Manual Review + Foundry

Recommendations

• To mitigate the impact of BASE_SKILL, the solution is straightforward. Navigate to the RapBattle implementation and adjust the BASE_SKILL value to 50.

• Note: The expected behavior of the RapBattle::getRapperSkill function is to increment each property of rappers based on the virtue they possess and also increment each property based on their vices improvements, hence the function also requires an update. Please see the suggested update below:

After updating the RapBattle constants and the RapBattle::getRapperSkill function, there is a significant change in the winning chances of both the defender and the challenger and the GAS is also minimized now.

Worst-case scenario with updated implementation:

• Defender with:

  • weakKnees: false

  • heavyArms: true

  • spaghettiSweater: true

  • calmAndReady: false

• Challenger with:

  • weakKnees: false

  • heavyArms: false

  • spaghettiSweater: false

  • calmAndReady: true

• Considering 65 as the unupdated BASE_SKILL and without changes in RapBattle::getRapperSkill function:

  • Defender's skill = 65 - 5 - 5 = 55

  • Challenger's skill = 65 + 10 = 75

  • Defender: 55 / 130 = 42.31% chance to win

  • Challenger: 75 / 130 = 57.69% chance to win

• Considering 50 as the updated BASE_SKILL with the updated RapBattle::getRapperSkill function:

  • Defender's skill = 50 + 5 + 0 + 0 + 0 = 55

  • Challenger's skill = 50 + 5 + 5 + 5 + 10 = 75

  • Defender: 55 / 130 = 42.31% chance to win

  • Challenger: 75 / 130 = 57.69% chance to win

- *Change in Winning Chance:*
- Defender's chances to win: 42.31 - 42.31 = 0%
- Challenger's chances to win: 57.69 - 57.69 = 0%

• Now, there are no changes in winning chances. The updated implementation reflects this lack of change. Therefore, our implementation is now correct. The grammatical error in the implementation led the implementer to implement a GAS-inefficient logic.

• By updating the RapBattle::getRapperSkill function, we have saved some GAS, as some verbose calculations are now eliminated. In the worst-case scenario, the RapBattle::getRapperSkill function had to perform two verbose calculations.