First Flight #10: One Shot
First Flight #10
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

The element `battlesWon` of the `RapperStats` struct is never used and updated

bube

Summary

The battlesWon from the struct IOneShot::RapperStats is never updated.

Vulnerability Details

The OneShot::updateRapperStats function is responsible for updating the struct RapperStats. This function can be called only from the Street contract:

function updateRapperStats(
uint256 tokenId,
bool weakKnees,
bool heavyArms,
bool spaghettiSweater,
bool calmAndReady,
uint256 battlesWon
) public onlyStreetContract {
RapperStats storage metadata = rapperStats[tokenId];
metadata.weakKnees = weakKnees;
metadata.heavyArms = heavyArms;
metadata.spaghettiSweater = spaghettiSweater;
metadata.calmAndReady = calmAndReady;
metadata.battlesWon = battlesWon;
}

The Streets contract calls the OneShot::updateRapperStats in unstake function and updates some of the parameters of the struct, but the battlesWon is not one of them:

function unstake(uint256 tokenId) external {
require(stakes[tokenId].owner == msg.sender, "Not the token owner");
//@audit-reported block.timestamp on Ethereum and Arbitrum can work differently
uint256 stakedDuration = block.timestamp - stakes[tokenId].startTime;
uint256 daysStaked = stakedDuration / 1 days;
​
// Assuming RapBattle contract has a function to update metadata properties
IOneShot.RapperStats memory stakedRapperStats = oneShotContract.getRapperStats(tokenId);
​
emit Unstaked(msg.sender, tokenId, stakedDuration);
delete stakes[tokenId]; // Clear staking info
​
// Apply changes based on the days staked
if (daysStaked >= 1) {
stakedRapperStats.weakKnees = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 2) {
stakedRapperStats.heavyArms = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 3) {
stakedRapperStats.spaghettiSweater = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 4) {
stakedRapperStats.calmAndReady = true;
credContract.mint(msg.sender, 1);
}
​
// Only call the update function if the token was staked for at least one day
if (daysStaked >= 1) {
oneShotContract.updateRapperStats(
tokenId,
stakedRapperStats.weakKnees,
stakedRapperStats.heavyArms,
stakedRapperStats.spaghettiSweater,
stakedRapperStats.calmAndReady,
stakedRapperStats.battlesWon
);
}

Impact

In the README is said:

Users mint a rapper that begins with all the flaws and self-doubt we all experience. NFT Mints with the following properties:
​
weakKnees - True
heavyArms - True
spaghettiSweater - True
calmandReady - False
battlesWon - 0
The only way to improve these stats is by staking in the Streets.sol

But the Streets contract doesn't update the battlesWon. And the battlesWon will always be 0 regardless how many wins has the rapper.

Tools Used

Manual Review

Recommendations

It is logical the battlesWon from struct RapperStats to be updated in the RapBattle::_battle function when someone wins a battle.
I know it is a design choice the OneShot::updateRapperStats to be updated only from the Streets contract. But consider the possibility for updating the RapperStats also from RapBattle contract in the _battle function or remove the battlesWon from RapperStats struct. It is not used and not updated.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`battlesWon` is never updated

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.