vesla0xfa - Pending deposits will always revert due to exceeded collateral capacity leading to a potential DoS

[sherlock-admin]

vesla0xfa

high

Pending deposits will always revert due to exceeded collateral capacity leading to a potential DoS

Summary

When vault's maximum collateral capacity is reached, pending deposit orders in the _announcedOrder pool will never be executed because they would exceed maximum vault's collateral capacity and the transaction will always revert. A malicious actor can exploit this scenario by closing the maximum capacity and spanning multiple non-executable transactions (using different EOA), with a high incentive to keepers, leading to a potential DoS of keepers.

Vulnerability Detail

When vault's maximum collateral capacity is being reached, users can still announce deposits. As mentioned in the docs, maximum vault's collateral capacity is $5M.

The Early Depositor Vault will have $5M maximum capacity. Once deposits reach $5M, no further deposits will be accepted.

To announce a deposit order, collateral tokens (rETH) are temporarily send to the DelayedOrder contract address and these funds will be locked until the order is executed or canceled.

We are considering a scenario where vault's collateral capacity is about to be reached. In this case, deposits can still be announced. So, a set of deposit orders, which add up the excess of vault's maximum capacity, could be announced and queued. As soon as some of these orders are executed and the maximum collateral capacity is reached, all the others will always revert.

Impact

1 - Keepers can be mislead spending gas trying to execute a transaction that will always revert; 2 - Bad actors could abuse this behavior and span deposit orders, using different EOAs, with a high incentive to keepers, leading to a potential DoS while these deposit orders live.

Code Snippet

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

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

PoC

function test_funds_locked() public {
    // setup scenario: Collateral cap is almost being reached.
    uint256 collateralPrice = 1000e8;
    uint256 maxCollateralCap = 5e24;
    uint256 amountLeftToCollateralCap = 1000e18;
    uint256 initialCollateralCap = maxCollateralCap - amountLeftToCollateralCap;
    uint256 keeperFee = mockKeeperFee.getKeeperFee();

    uint256 aliceBalanceBefore = WETH.balanceOf(alice);
    uint256 aliceDepositAmount = 500e18;

    vm.startPrank(admin);
    vaultProxy.setStableCollateralCap(maxCollateralCap);
    announceAndExecuteDeposit({
        traderAccount: admin,
        keeperAccount: keeper,
        depositAmount: initialCollateralCap,
        oraclePrice: collateralPrice,
        keeperFeeAmount: 0
    });

    // Alice tries to deposit funds.
    // Alice's order was not executed immediately because
    // she offered a small fee to keepers.
    announceStableDeposit({
        traderAccount: alice,
        depositAmount: aliceDepositAmount,
        keeperFeeAmount: 0
    });

    // Alice's funds are temporarilly transfered to DelayedOrderModule and
    // will be transfered to the FlatcoinVault when it's executed.
    assertEq(WETH.balanceOf(alice), aliceBalanceBefore - aliceDepositAmount - keeperFee);

    // A malicious actor could flood several deposit orders with different EOA
    // and high incentive to keepers, leading to a potential DOS
    announceStableDeposit({
        traderAccount: foo,
        depositAmount: 1e6,
        keeperFeeAmount: keeperFee * 100
    });

    announceStableDeposit({
        traderAccount: bar,
        depositAmount: 1e6,
        keeperFeeAmount: keeperFee * 100
    });

    // Bob creates an order and uses his own keeper implementation to instantly.
    // execute it. So, his order "frontruns" Alice's and the maximum stable collateral
    // capacity is reached.
    announceAndExecuteDeposit({
        traderAccount: bob,
        keeperAccount: keeper,
        depositAmount: amountLeftToCollateralCap,
        oraclePrice: collateralPrice,
        keeperFeeAmount: 0
    });

    // At this point, keepers would be prioritizing orders with a high incentive fee
    // incentive fee (i.e. those created by foo and bar), leading to a potential DoS,
    // scenario, while these orders are not expired.

    // Alice's funds will be locked until her order is manually cancelled.
    vm.startPrank(alice);
    bytes[] memory priceUpdateData = getPriceUpdateData(collateralPrice);

    vm.expectRevert(
        abi.encodeWithSelector(FlatcoinErrors.DepositCapReached.selector, maxCollateralCap)
    );
    delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);

    // NOTE: Alice should await her order to expire and cancel it
    // manually because:
    // 1 - there is no way to cancel an order before it is expired;
    // 2 - there is no incentives for keepers cancel orders, since
    // the fees are returned to keepers when an order is cancelled.
    vm.expectRevert(FlatcoinErrors.OrderHasNotExpired.selector);
    delayedOrderProxy.cancelExistingOrder(alice);

    // Alice waits
    skip(1.5 minutes); 

    // Alice can retrieve her tokens
    delayedOrderProxy.cancelExistingOrder(alice);
    assertEq(WETH.balanceOf(alice), aliceBalanceBefore);

   // Bob can withdraw his money, opening the vault maximum capacity,
   // and repeat the described process of spanning non-executable deposit orders.
}

filipealves@vesla01 in ~/Developer/audits/2023-12-flatmoney-filipealvesdef/flatcoin-v1 on main [!?]
[i] $ forge test --match-test test_funds_locked
[⠆] Compiling...
[⠒] Compiling 1 files with 0.8.18
[⠆] Solc 0.8.18 finished in 8.70s
Compiler run successful!

Running 1 test for test/unit/Stable-Module/Deposit.t.sol:DepositTest
[PASS] test_funds_locked() (gas: 2000720)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 20.38ms

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

Tool used

vim, Foundry

Recommendation

Implement a tracking for pendingDepositAmount. Check it in DelayedOrder.sol::announceStableDeposit at line 74, update when the deposit is executed and canceled to avoid non-executable deposit orders being created.

[sherlock-admin]

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

takarez commented:

invalid: the recommendation is not of any diffPercent compared to the reverting in case of execeeding the max limit

[nevillehuang]

Low severity, since max collateral cap can always be adjusted here

[vesla0x1]

Escalate

In my opinion, the issue represents a risk because at the moment when maxCollateralCap is almost reached, there is a window of opportunity to this happen. Perhaps my first explanation was not clear enough, so I will try to describe it better:

Even though the maximum collateral limit can be adjusted, once maxCollateralCap is almost reached, there is a window of opportunity for a malicious agent to first announce multiple deposit orders with a high incentive for keepers (using different EOAs), and then announce and execute a final deposit order responsible for reaching the maxCollateralCap. From this moment on, the execution of deposit orders will always revert, and if there are multiple pending deposit orders with high incentives for keepers, they will be prioritized over all other existing or newly announced orders, even those with potential for execution (i.e., withdrawal, opening, adjusting, and closing positions).

Therefore, regardless of the value of maxCollateralCap being adjustable, as long as the problem is not detected and maxCollateralCap is not increased, keepers will prioritize a set of orders that will always revert, resulting in:

1. Orders from traders with potential for execution not being executed by keepers while the maximum collateral capacity has been reached, and these deposit orders live in the pool, leading to losses or missed opportunities for profitable trades by traders.
2. Gas griefing of keepers. The higher the number of injected deposit orders, the worse.
3. Temporarily locked funds from LPs that announced deposit orders before maxCollateralCap was reached - In this case, the LP would have to wait for its deposit order to expire and manually cancel it, as it is not possible to cancel unexpired orders and keepers do not receive incentives to execute order cancellations. NOTE: I know that issues related to temporarily locked funds are not considered valid, but I still found it relevant to mention.

Someone might argue that pending orders have a short lifetime, and after these deposit orders expire, keepers will no longer attempt to execute them. This is correct, but the malicious agent, who made the deposit responsible for reaching maxCollateralCap, could withdraw his deposited funds, opening the window of opportunity again and repeating the described process.

In my opinion, this issue should be considered, especially in this Season 1 of Early Depositors, which, as described in the documentation, has a maximum collateral capacity for the vault of $5M, and exploring this scenario is feasible.

[rashtrakoff]

Even though the maximum collateral limit can be adjusted, once maxCollateralCap is almost reached, there is a window of opportunity for a malicious agent to first announce multiple deposit orders with a high incentive for keepers (using different EOAs), and then announce and execute a final deposit order responsible for reaching the maxCollateralCap.From this moment on, the execution of deposit orders will always revert, and if there are multiple pending deposit orders with high incentives for keepers, they will be prioritized over all other existing or newly announced orders, even those with potential for execution (i.e., withdrawal, opening, adjusting, and closing positions).

All of this assumes keepers aren't simulating the transaction reversion status. Keepers should simulate the transactions as there are a few ways to create unexecutable orders.

I don't think there is anything we can do apart from actually tracking deposit amounts when an order is announced. This is not a good idea as the keepers may not prioritise based on FCFS method.