Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenRegistryHelpers.sol#L119-L124

Vulnerability details

Impact

The potential impact of this issue includes:

Unauthorized Token Withdrawal: Attackers could exploit this vulnerability to perform unauthorized token withdrawals by causing precision loss in balance calculations.
Financial Losses: Users and the contract itself could suffer financial losses if tokens are withdrawn incorrectly.
Disruption of Contract Logic: The precision loss can disrupt the intended logic of the contract by allowing balances to wrap around to large positive values instead of becoming negative as expected.

Vulnerability Details

function calculateDecreasedAmount

function calculateDecreasedAmount(address delegateRegistry, bytes32 registryHash, uint256 decreaseAmount) internal view returns (uint256) {
    unchecked {
        return
            uint256(IDelegateRegistry(delegateRegistry).readSlot(bytes32(uint256(RegistryHashes.location(registryHash)) + RegistryStorage.POSITIONS_AMOUNT))) - decreaseAmount;
    }
}

In the calculateDecreasedAmount function, decreaseAmount is subtracted from the existing value read from storage. If decreaseAmount is greater than the existing value, it would lead to an underflow, which, in the context of unsigned integers (like uint256), wraps around and results in a very large value that may not make sense in the application's logic.

Proof of Concept

The calculateDecreasedAmount function subtracts a specified decreaseAmount from an existing value read from storage. If decreaseAmount is greater than the existing value, it can lead to an underflow, causing precision loss and potentially unintended behavior.

Let's say the existing value read from storage is 100 (just as an example), and an attacker sends a transaction with decreaseAmount set to 200. The calculation would look like this:

New Value = 100 - 200 = 2^256 - 100

The result of this subtraction operation would be a very large positive integer due to underflow.

Tools Used

Manual

Recommended Mitigation Steps

Add a check to ensure that decreaseAmount is not greater than the existing value before performing the subtraction. You can use a require statement for this purpose.

function calculateDecreasedAmount(address delegateRegistry, bytes32 registryHash, uint256 decreaseAmount) internal view returns (uint256) {
    unchecked {
        uint256 currentAmount = uint256(IDelegateRegistry(delegateRegistry).readSlot(bytes32(uint256(RegistryHashes.location(registryHash)) + RegistryStorage.POSITIONS_AMOUNT)));
        require(decreaseAmount <= currentAmount, "Decrease amount exceeds current amount");
        return currentAmount - decreaseAmount;
    }
}

With this now, if decreaseAmount is greater than the existing value, the function will revert, preventing an underflow and ensuring that precision is not lost.

Assessed type

Under/Overflow

code-423n4 / 2023-09-delegate-findings

Precision Loss in Subtraction (Integer Underflow). #208