Arithmetic underflow/overflow when deposit amount is +- 1.157e77

Summary

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L524-L568

Arithmetic underflow/overflow when deposit amount is around 115792089237316195423570985008687907853269984665640564039457584007913129639933

Vulnerability Detail

$ forge test --contracts test/unit/Delayed-Order/DelayedOrder.t.sol --mt testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero -vvv
[⠒] Compiling...
[⠑] Compiling 1 files with 0.8.18
[⠃] Solc 0.8.18 finished in 9.09s
Compiler run successful!

Running 1 test for test/unit/Delayed-Order/DelayedOrder.t.sol:DelayedOrderTest
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11); counterexample: calldata=0x791d65540000000000000000000000000000000000000000000000004544b169e96a87450000000000000000000000000000000000000000000000003782dace9d900000 args=[4991309355479107397 [4.991e18], 4000000000000000000 [4e18]]] testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256,uint256) (runs: 0, μ: 0, ~: 0)
Traces:
  [37094] DelayedOrderTest::testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(4991309355479107397 [4.991e18], 4000000000000000000 [4e18])
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   └─ ← ()
    ├─ [2585] ERC20::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [staticcall]
    │   └─ ← 100000000000000000000000 [1e23]
    ├─ [9715] TransparentUpgradeableProxy::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [staticcall]
    │   ├─ [2576] StableModule::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [delegatecall]
    │   │   └─ ← 0
    │   └─ ← 0
    ├─ [3089] TransparentUpgradeableProxy::stableCollateralPerShare() [staticcall]
    │   ├─ [2453] StableModule::stableCollateralPerShare() [delegatecall]
    │   │   └─ ← 1000000000000000000 [1e18]
    │   └─ ← 1000000000000000000 [1e18]
    └─ ← panic: arithmetic underflow or overflow (0x11)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 49.23ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/unit/Delayed-Order/DelayedOrder.t.sol:DelayedOrderTest
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11); counterexample: calldata=0x791d65540000000000000000000000000000000000000000000000004544b169e96a87450000000000000000000000000000000000000000000000003782dace9d900000 args=[4991309355479107397 [4.991e18], 4000000000000000000 [4e18]]] testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256,uint256) (runs: 0, μ: 0, ~: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

ANOTHER TEST WITH DIFFERENT DEPOSIT, ALTHOUGH THIS IS MORE THE EXCEPTION:

  [37094] DelayedOrderTest::testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(27455050074662375853 [2.745e19], 4000000000000000000 [4e18])
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   └─ ← ()
    ├─ [2585] ERC20::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [staticcall]
    │   └─ ← 100000000000000000000000 [1e23]
    ├─ [9715] TransparentUpgradeableProxy::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [staticcall]
    │   ├─ [2576] StableModule::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [delegatecall]
    │   │   └─ ← 0
    │   └─ ← 0
    ├─ [3089] TransparentUpgradeableProxy::stableCollateralPerShare() [staticcall]
    │   ├─ [2453] StableModule::stableCollateralPerShare() [delegatecall]
    │   │   └─ ← 1000000000000000000 [1e18]
    │   └─ ← 1000000000000000000 [1e18]
    └─ ← panic: arithmetic underflow or overflow (0x11)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 47.96ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/unit/Delayed-Order/DelayedOrder.t.sol:DelayedOrderTest
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11); counterexample: calldata=0x791d65540000000000000000000000000000000000000000000000017d03ee946ed011ad0000000000000000000000000000000000000000000000003782dace9d900000 args=[27455050074662375853 [2.745e19], 4000000000000000000 [4e18]]] testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256,uint256) (runs: 0, μ: 0, ~: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

Impact

Reverts or panics, tx doesn't complete.

Code Snippet

    /// @notice User delayed withdrawal from the stable LP.
    /// @dev Uses the Pyth network price to execute.
    /// @param account The user account which has a pending withdrawal.
    /// @return amountOut The amount of collateral asset tokens the user receives.
    function _executeStableWithdraw(address account) internal returns (uint256 amountOut) {
        FlatcoinStructs.Order memory order = _announcedOrder[account];

        _prepareExecutionOrder(account, order.executableAtTime);

        FlatcoinStructs.AnnouncedStableWithdraw memory stableWithdraw = abi.decode(
            order.orderData,
            (FlatcoinStructs.AnnouncedStableWithdraw)
        );

        uint256 withdrawFee;

        (amountOut, withdrawFee) = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY))
            .executeWithdraw(account, order.executableAtTime, stableWithdraw);

        uint256 totalFee = order.keeperFee + withdrawFee;
        //uint256 totalFee = amountOut; /// //audit added for PoC/testing purposes

        // Make sure there is enough margin in the position to pay the keeper fee and withdrawal fee
        if (amountOut < totalFee) revert FlatcoinErrors.NotEnoughMarginForFees(int256(amountOut), totalFee);

        // include the fees here to check for slippage
        amountOut -= totalFee;
        //amountOut = 0; /// //audit added for PoC/testing purposes
        assert(amountOut > 0); /// //audit added for PoC/testing purposes

        emit TestOutput(amountOut, totalFee); /// //audit added for PoC/testing purposes

        if (amountOut < stableWithdraw.minAmountOut)
            revert FlatcoinErrors.HighSlippage(amountOut, stableWithdraw.minAmountOut);

        // Settle the collateral
        vault.updateStableCollateralTotal(int256(withdrawFee)); // pay the withdrawal fee to stable LPs
        vault.sendCollateral({to: msg.sender, amount: order.keeperFee}); // pay the keeper their fee
        uint256 vaultBalanceOf_account_before = vault.collateral().balanceOf(account); /// //audit added for PoC/testing purposes
        vault.sendCollateral({to: account, amount: amountOut}); // transfer remaining amount to the trader
        uint256 vaultBalanceOf_account_after = vault.collateral().balanceOf(account); /// //audit added for PoC/testing purposes
        emit VaultBalanceOfAccount(vaultBalanceOf_account_before, vaultBalanceOf_account_after); /// //audit added for PoC/testing purposes /// //audit added for VaultBalanceOfAccount

        emit FlatcoinEvents.OrderExecuted({account: account, orderType: order.orderType, keeperFee: order.keeperFee});
    }

TEST FUNCTION:

    /// //audit test function for PoC/testing purposes
    //function testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256 _depositAmount, uint256 _withdrawAmount) public {
    ///function testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256 _depositAmount, uint256 _withdrawAmount, uint256 _keeperFeeAmount) public {
    function testFuzz_NOT_revert_execute_withdraw_when_remaining_amountOut_is_zero(uint256 _withdrawAmount, uint256 _keeperFeeAmount) public {
        uint256 oraclePrice = 1000e8;

        uint256 _depositAmount = 115792089237316195423570985008687907853269984665640564039457584007913129639933;
        //uint256 _depositAmount = 1157920892373161954235700;
        vm.assume(_depositAmount >= 1e6 && _withdrawAmount != 0 && _keeperFeeAmount >= 0); /// //audit added for PoC/testing purposes
        //vm.assume(_depositAmount >= 1e6 && _withdrawAmount != 0); /// //audit added for PoC/testing purposes
        //vm.assume(_withdrawAmount >= 19999999999999998 && _withdrawAmount <= _depositAmount); /// //audit added for PoC/testing purposes
        vm.assume(_withdrawAmount >= _keeperFeeAmount && _withdrawAmount <= _depositAmount); /// //audit added for PoC/testing purposes
        //vm.assume(_keeperFeeAmount <= 2e16); /// //audit added for PoC/testing purposes
        vm.assume(_keeperFeeAmount >= 4e18 && _keeperFeeAmount <= 50e18);

        announceAndExecuteDeposit({
            traderAccount: alice,
            keeperAccount: keeper,
            //depositAmount: 1e18,
            depositAmount: _depositAmount, /// //audit added for PoC/testing purposes
            oraclePrice: oraclePrice,
            //keeperFeeAmount: 19999999999999998
            keeperFeeAmount: _keeperFeeAmount /// //audit added for PoC/testing purposes
        });

        //announceStableWithdraw({traderAccount: alice, withdrawAmount: 1e18, keeperFeeAmount: 0});
        //announceStableWithdraw({traderAccount: alice, withdrawAmount: 19999999999999999, keeperFeeAmount: 19999999999999998}); /// //audit added for PoC/testing purposes
        announceStableWithdraw({traderAccount: alice, withdrawAmount: _withdrawAmount, keeperFeeAmount: _keeperFeeAmount}); /// //audit added for PoC/testing purposes
        skip(1 minutes);

        vm.startPrank(keeper);

        bytes[] memory priceUpdateData = getPriceUpdateData(oraclePrice);

        _expectRevertWithCustomError({
            target: address(delayedOrderProxy),
            callData: abi.encodeWithSelector(delayedOrderProxy.executeOrder.selector, alice, priceUpdateData),
            expectedErrorSignature: "AmountOut cannot be ZERO!", /// //audit change this error to just string/text if necessary
            ignoreErrorArguments: true,
            value: 1
        });

    }

Tool used

Foundry's fuzzer. Manual Review

Recommendation

Needs further investigation as I didn't have enough time to get to the bottom of why this is happening as well as overall impacts on the protocol & users.

sherlock-admin commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

nevillehuang commented 4 months ago

Invalid, deposit amounts will not reach that kind of level given rETH supply. For your info that number is uint256.max

dappconsulting commented 4 months ago

Fair enough if deposit amounts can never reach that kind of level given rETH supply, so no risk to protocol then.

And thanks for pointing out that the number that was causing all the reverts during fuzzing is actually uint256.max. Makes sense now. 👀

sherlock-audit / 2023-12-flatmoney-judging

JP_Courses - Arithmetic underflow/overflow when deposit amount is +- 1.157e77 #283