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
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.
Let's analyze a rap battle scenario with a bet of 1 cred, considering the worst-case scenario with the current 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 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
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.
Manual Review + Foundry
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.
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.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.