Bjorn_Bug - msg.sender Can Only Announce One Order At a Time

[sherlock-admin2]

Bjorn_Bug

medium

msg.sender Can Only Announce One Order At a Time

Summary

Users can place only one order at a time because the _prepareAnnouncementOrder() function, used in each announce Order function, prevents creating multiple announce orders for the same msg.sender in the DelayedOrder contract

Vulnerability Detail

When msg.sender calls announceStableDeposit() to announce his first order, the _prepareAnnouncementOrder() function is called internally. This function then calls cancelExistingOrder() to check if msg.sender has any expired orders to cancel before announcing a new one. cancelExistingOrder() is a public function that starts with two importants checks:

function cancelExistingOrder(address account) public {

        FlatcoinStructs.Order memory order = _announcedOrder[account];

        if (order.orderType == FlatcoinStructs.OrderType.None) return;

        if (block.timestamp <= order.executableAtTime + vault.maxExecutabilityAge())
            revert FlatcoinErrors.OrderHasNotExpired();

Since the first announceStableDeposit order for msg.sender hasn't been created yet, cancelExistingOrder() will simply return here:

if (order.orderType == FlatcoinStructs.OrderType.None) return;

And then the transaction will continue, resulting in the creation of announceStableDeposit.

Just after the first transaction is complete, if the same msg.sender calls announceStableDeposit again (to add more liquidity), The following will happen: announceStableDeposit() is invoked, which then calls cancelExistingOrder(). As I mentionned above cancelExistingOrder() function checks if msg.sender has announced order first, in this case the first check passes (the first order) and moves to the second check to verify if the first anounced order is expired to cancel it because creating a second one.

if (block.timestamp <= order.executableAtTime + vault.maxExecutabilityAge())
            revert FlatcoinErrors.OrderHasNotExpired();

This check will fail and announceStableDeposit() reverts because the first order hasn't expired yet. As a result, a liquidity provider can't announce a second stable deposit order or add more liquidity until their first order is executed or canceled.

POC

Add the test bellow to Leverage.t.sol, and running the test will cause it to revert with OrderHasNotExpired()

function test_announceDeposit_two() public {

        announceStableDeposit({
            traderAccount: alice, 
            depositAmount: 100e18,
            keeperFeeAmount: 0 
        });

        announceStableDeposit({
            traderAccount: alice, 
            depositAmount: 100e18,
            keeperFeeAmount: 0 
        });
}

Impact

This issue can cause a Denial of Service (DOS) for users by restricting them to only announcing one order at a time, preventing full participation in various protocol functionalities (e.g., Stable Deposit, Open Leverage) simultaneously.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L67 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L72 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L634 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L644 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L421-L422

Tool used

Manual Review

Recommendation

I recommend removing the cancelExistingOrder() function from _prepareAnnouncementOrder() to allow users to announce more than one order at time. you can also introduce a mechanism that encourages users to cancel their expired orders by themselves, maybe by gaining or losing points based on the number of expired orders they cancels.

[sherlock-admin]

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

takarez commented:

invalid: thats intended behavior

[nevillehuang]

Invalid and related to #207, and by design, wherein admins are trusted to adjust and set appropriate executability age to allow announcement of new orders.