Overflow Vulnerability in Smart Contract Budget Allocation Mechanism

[hats-bug-reporter[bot]]

Github username: @hama-tech Twitter username: -- Submission hash (on-chain): 0x05457d8c4fed52301f25fa04a95a53e253e32169f9caca76dd7ed7801085d8dc Severity: medium

Description: Description\

The identified vulnerability pertains to potential overflow risks in a Solidity smart contract, specifically within the notifyDistributedQuestPeriod function during the calculation of palAmount and extraAmount. The issue arises from downcasting the result of multiplication operations to uint128, posing a risk of overflow if the calculated values surpass the maximum representable value for uint128.

Impact:

Incorrect Budget Allocation: The overflow risk may result in inaccuracies in calculated increments, causing discrepancies in the intended budget allocations and impacting reward distribution.

Smart Contract Vulnerability: In extreme scenarios, overflow could introduce vulnerabilities, including the potential for reentrancy attacks or other unpredictable behaviors. Such vulnerabilities may compromise the overall integrity of the smart contract.

Attack Scenario\

The vulnerability occurs when the product of budget.palAmount unsunedWeight or budget.extraAmount unsunedWeight is sufficiently large such that casting the result to uint128 may overflow, losing the higher-order bits and leading to an incorrect value being stored.

        pengingBudget.palAmount += uint128(uint256(budget.palAmount) * unsunedWeight / UNIT);
        pengingBudget.extraAmount += uint128(uint256(budget.extraAmount) * unsunedWeight / UNIT);

Attachments

1. Proof of Concept (PoC) File

https://github.com/hats-finance/Paladin-0x1610bfde27e57b068af7f38aec3d2a7b1d146989/blob/cf3c82f102a76f58acf003980c480eb9028f0e94/contracts/LootCreator.sol#L319 https://github.com/hats-finance/Paladin-0x1610bfde27e57b068af7f38aec3d2a7b1d146989/blob/cf3c82f102a76f58acf003980c480eb9028f0e94/contracts/LootCreator.sol#L320

2. Revised Code File (Optional)

   Explicit Checks for Overflow: Implement explicit checks to ensure that the result of multiplication and division operations does not exceed the maximum value representable by uint128. This helps prevent unintended overflow.

[Kogaroshi]

This downcasting is made this way since the budget.palAmount is an uint128 at the start, and the value unsunedWeight (typo will be fixed) cannot be more than 1e18, since it's based on the following calculation : (pointsWeight[gauge][ts].bias * UNIT) / _totalWeight; based on the bias of a gauge at a given period, and the total bias of all gauge, so with UNIT (being 1e18) in this calculation, it implies that the result (the gauge relative weight, and so the unused part in case it's over the cap) will never be over UNIT (never more than 1e18). So the calculation pengingBudget.palAmount += uint128(uint256(budget.palAmount) * unsunedWeight / UNIT); start with a uint128, which is multiplied by 1e18, and divided by at max 1e18, or less, which means the value at the end can safely be casted back to an uint128

[hama-tech]

that was just an example, here duration is uint256

https://github.com/hats-finance/Paladin-0x1610bfde27e57b068af7f38aec3d2a7b1d146989/blob/cf3c82f102a76f58acf003980c480eb9028f0e94/contracts/HolyPalPower.sol#L177

and downcasted to uint128

https://github.com/hats-finance/Paladin-0x1610bfde27e57b068af7f38aec3d2a7b1d146989/blob/cf3c82f102a76f58acf003980c480eb9028f0e94/contracts/HolyPalPower.sol#L180

[Kogaroshi]

Here too it was made on purpose since we know the duration of a hPAL Lock is maximum 2 years, so the value should always be small enough for a conversion to uint128 without any issue.