DelegateTokenRegistryHelpers::calculateDecreasedAmount() - Comment "Assumes the decreased amount won't underflow with "amount"", but it *can* underflow given the right value for parameter `decreaseAmount`, and is also inside unchecked {} block.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/69a3bd06f2f0e051c5438946609fe1fb41b7c264/src/libraries/DelegateTokenRegistryHelpers.sol#L112-L124 https://github.com/code-423n4/2023-09-delegate/blob/69a3bd06f2f0e051c5438946609fe1fb41b7c264/src/libraries/DelegateTokenRegistryHelpers.sol#L122

Vulnerability details

Impact

Summary:

Since the affected line is inside an unchecked block, the inbuilt solidity protection functionality wont revert this function execution on underflow, therefore the max uint256 value will be returned for calculateDecreasedAmount().

Not only that, but the following functions can/will be affected in such a scenario if they depend on this value during the same time:

transferERC20(), transferERC1155(), decrementERC20(), decrementERC1155()

Proof of Concept

Affected code:

    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;
        }
    }

Tools Used

VSC.

Recommended Mitigation Steps

Recommendation:

Either add an input validation check to ensure decreaseAmount cannot be bigger than amount, or remove the unchecked {} block.

Assessed type

Under/Overflow

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[GalloDaSballo]

We read and then update, missing proof that this can be performed

Please ALWAYS provide an instance of the bug for math type findings