Voltage refresh based only on block.timestamp check leads to unnecessary update of existing amount of voltage in case of having sufficient amount of it or using a battery

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L105-L112

Vulnerability details

Impact

_replenishVoltage() function, which sets user's voltage to 100, can only be called by the spendVoltage() function when the following if-check passes:

        if (ownerVoltageReplenishTime[spender] <= block.timestamp)

This function doesn't check whether a user has enough voltage to spend or not, the only thing that is considered here is the possibility of replenishing voltage based on the condition if the enough amount of time has passed since the last replenishing time. Consequences are as follows:

1. If the full amount of voltage wasn't spent during the day, the rest of it will be overwritten to 100, then the amount of voltageSpent will be deducted from the ownerVoltage[spender] amount even if the user had enough voltage at the moment when this function was called, which leads to unnecessary loss of voltage for users. The only way voltage can be replenished is by buying a VoltageBattery, which requires users to spend some amount of $NRN on it.
2. Proceeding from the provided implementation, I assume that it is supposed that users will always keep in mind the time when their voltage can be replenished but this condition most likely won't be always met. This leads to a situation when users may use their VoltageBattery shortly before their ownerVoltageReplenishTime[spender] will actually pass the if-check condition of the spendVoltage() function so there will be no way for them to take not only full but even small advantage of their spent VoltageBattery, making its usage useless and therefore resulting in loss of $NRN, since previously VoltageBattery should have been bought.
3. This also may happen not in case of the user's fault, but because of the Arbitrum's Sequencer going down for some period of time. Consider the following scenario: player uses his VoltageBattery in order to continue playing, sequencer goes down for N amount of time, during which his ownerVoltageReplenishTime[spender] becomes less or equal to the current block.timestamp, so in the next call to the spendVoltage() function his energy will be replenished, which leads to VoltageBattery being spent without any impact.

   Details

   There are two similar implementations of this logic in the protocol, one of which can be found in the GameItems.sol contract and the other of VoltageManager.sol is already illustrated in the previous section of the report. Let's compare them and see why the same implementation is suitable for the first contract and not so fitting for the second.

4. In the GameItems.sol contract there is an if-check for dailyAllowanceReplenishTime[msg.sender][tokenId] in the mint() function, which represents the possibility to replenish the amount of items a user can mint daily:

               if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
                   _replenishDailyAllowance(tokenId);
               }

   When the item is created in the createGameItem() function there is a dailyAllowance parameter, which determines the amount of certain item that can be minted in a day, therefore any violation of this condition will actually break the logic of the protocol, that is why in the GameItems.sol contract we do not care, whether a user has reached the limit of possible minted amount of the item during the day or not. Possible minted amount of the item is limited by the dailyAllowance parameter in the GameItemAttributes struct.

   2. The situation with voltage in the VoltageManager.sol contract is different. We do not have any daily limitation of voltage, since it is always possible to refill it with the VoltageBattery as many times as user wants, there is only a cap on the upper limit set to 100. By following the same logic applied to items in the GameItems.sol contract, protocol basically limits voltage as it was an item with its dailyAllowance set to 100, but this is not the case. If a user wasn't able to play a game for 1 day, he would literally lose his preserved 100 voltage without any reason, whereas he should have had an ability to firstly spend the amount voltage that he had and only then to replenish it to 100 again, following the logic of the voltage energy-like concept. Moreover, by limiting the amount of voltage protocol actually forces users to buy VoltageBattery, whilst they could have just used their existing sufficient amount of voltage.

Proof of Concept

The way current implementation works is well illustrated in the given VoltageManager.t.sol test suite in the testSpendVoltageTriggerReplenishVoltage() and testSpendVoltage() functions.

Tools used

Manual Review, Foundry

Recommended Mitigation Steps

Update the spendVoltage function in the VoltageManager.sol contract in the following way:

    function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
        uint8 voltageLeft = ownerVoltage[spender]; //@audit add this variable
        if (voltageLeft < voltageSpent) { //@audit add this check
            if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            _replenishVoltage(spender);
          }
        }
        ownerVoltage[spender] -= voltageSpent;
        emit VoltageRemaining(spender, ownerVoltage[spender]);
    }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #27

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)