Summary

The logic in the unstake() function in Street.sol causes unnecessary fees for users. Any user who has staked for more than 1 day will end up performing multiple credContract.mint() calls when only 1 is necessary. This will cause higher gas fees for the users. To the best of my knowledge, there is no benefit in implementing the logic in this manner, and it is a purely unnecessary cost.

Vulnerability Details

Here is the unstake() function in Street.sol.

// Unstake tokens by transferring them back to their owner
function unstake(uint256 tokenId) external {
    require(stakes[tokenId].owner == msg.sender, "Not the token owner");
    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
        );
    }

    // Continue with unstaking logic (e.g., transferring the token back to the owner)
    oneShotContract.transferFrom(address(this), msg.sender, tokenId);
}

Look at how the if statements are structured. They are structured 1 - 4, and each if statement that is triggered causes a mint() transaction. Instead, the if statements should be structured 4-1 with each only performing a single mint transaction with the appropriate number of tokens. Also, else statements should be used so we can exit the logic early if possible. Here is an implementation of the fixed unstake() function.

// Unstake tokens by transferring them back to their owner
function unstake(uint256 tokenId) external {
    require(stakes[tokenId].owner == msg.sender, "Not the token owner");
    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 >= 4) {
        stakedRapperStats.calmAndReady = true;
        stakedRapperStats.spaghettiSweater = false;
        stakedRapperStats.heavyArms = false;
        stakedRapperStats.weakKnees = false;
        credContract.mint(msg.sender, 4);
    }
    else if (daysStaked == 3) {
        stakedRapperStats.spaghettiSweater = false;
        stakedRapperStats.heavyArms = false;
        stakedRapperStats.weakKnees = false;
        credContract.mint(msg.sender, 3);
    }
    else if (daysStaked == 2) {
        stakedRapperStats.heavyArms = false;
        stakedRapperStats.weakKnees = false;
        credContract.mint(msg.sender, 2);
    }else if(daysStaked == 1){
        stakedRapperStats.weakKnees = false;
        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
        );
    }

    // Continue with unstaking logic (e.g., transferring the token back to the owner)
    oneShotContract.transferFrom(address(this), msg.sender, tokenId);
}

Note that the last if statement [ if (daysStaked >= 1) ] can also be nested in the other if statements if even lower costs are desired.

Impact

Poor logic implementation leads to unnecessary costs.

Tools Used

Foundry

Recommendations

Implement the logic in such a manner to minimize the computation performed and the transactions performed.