search

code-423n4 / 2023-03-polynomial-findings

2 stars 1 forks source link

Malicious users can exploit deposit and withdrawal queueing in KangarooVault and LiquidityPool contracts to force exorbitant transaction fees #123

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L185-L334 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L181-L335

Vulnerability details

Impact

Direct loss of money for users who have deposited funds and wish to withdraw them, as they would be required to pay extremely high gas fees due to the queuing mechanism exploited by the attacker.

Proof of Concept

The affected contracts, KangarooVault and LiquidityPool, provide the functionality for instant deposits and withdrawals of tokens. However, in certain scenarios, deposits and withdrawals can be queued.

In both cases, the deposit transfers the sUSD from your address immediately. In the instant deposit scenario,mints you Vault or Liquidity token - depending on the contract. On the other hand, queuing it adds you to the deposit queue, by creating a "QueuedDeposit" object and sets its id to the current value of nextQueuedDepositId (mentioning that, because its going to be important later)

        QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];
        newDeposit.id = nextQueuedDepositId++;
        newDeposit.user = user;
        newDeposit.depositedAmount = amount;
        newDeposit.requestedTime = block.timestamp;
        totalQueuedDeposits += amount;

In KangarooVault, the deposits are queued ONLY If there are currently active positions.

    function initiateDeposit(address user, uint256 amount) external nonReentrant {
        require(user != address(0x0));
        require(amount >= minDepositAmount);

        // Instant processing
        if (positionData.positionId == 0) {
            uint256 tokenPrice = getTokenPrice();
            uint256 tokensToMint = amount.divWadDown(tokenPrice);
            VAULT_TOKEN.mint(user, tokensToMint);
            totalFunds += amount;
            emit ProcessDeposit(0, user, amount, tokensToMint, block.timestamp);
        } else {
            // Queueing the deposit request
            QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];

            newDeposit.id = nextQueuedDepositId++;
            newDeposit.user = user;
            newDeposit.depositedAmount = amount;
            newDeposit.requestedTime = block.timestamp;

            totalQueuedDeposits += amount;
            emit InitiateDeposit(newDeposit.id, msg.sender, user, amount);
        }

        // SUSD checks for zero address
        SUSD.safeTransferFrom(msg.sender, address(this), amount);
    }

On the other hand, in LiquidityPool instant deposits have a fee whereas the regular deposits don't have a fee. After discussing with the protocol team, queue deposits its primarily for the regular traders, because the price won't fluctate that much and instant is mostly for other protocols to build on top.

    function queueDeposit(uint256 amount, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_DEPOSIT")
    {
        QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];
        newDeposit.id = nextQueuedDepositId++;
        newDeposit.user = user;
        newDeposit.depositedAmount = amount;
        newDeposit.requestedTime = block.timestamp;
        totalQueuedDeposits += amount;
        SUSD.safeTransferFrom(msg.sender, address(this), amount);

        emit InitiateDeposit(newDeposit.id, msg.sender, user, amount);
    }

Finally, the deposits are processed in a nearly identical way (LP, Kangaroo). Below, I've only extracted the vulnerable code which contains a for loop that iterates through a specified number of deposits, denoted by the variable count. The main purpose of this loop is to process each deposit in the queue and update the overall state of the contract.

At the beginning of each iteration, the function accesses the deposit at the current queuedDepositHead position in the depositQueue. The deposit's requestedTime is then evaluated to ensure that it is not equal to 0 and that the current block timestamp exceeds the deposit's requestedTime plus the minDepositDelay. If either of these conditions is true, the function terminates early, halting further deposit processing.

Upon passing the aforementioned check, it mints the tokens for the user and updates the contract accounting balance. Finally, the queuedDepositHead is incremented, advancing to the next deposit in the queue.

We can conclude that:

As a result, this means all the deposits are executed in sequential order and if there are currently 10 deposits and yours is the 11th, if you want to process your own, you have to process the 10 before that or wait for somebody else to process them.

      for (uint256 i = 0; i < count; i++) {
            QueuedDeposit storage current = depositQueue[queuedDepositHead];

            if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {
                return;
            }

            uint256 tokensToMint = current.depositedAmount.divWadDown(tokenPrice);

            current.mintedTokens = tokensToMint;
            totalQueuedDeposits -= current.depositedAmount;
            totalFunds += current.depositedAmount;
            liquidityToken.mint(current.user, tokensToMint);

            emit ProcessDeposit(current.id, current.user, current.depositedAmount, tokensToMint, current.requestedTime);

            current.depositedAmount = 0;
            queuedDepositHead++;
        }

The queuing mechanism can be exploited by a malicious actor, who can queue a large number of deposits or withdrawals for a very low cost. This essentially locks all the deposited funds in the contract and forces the users to pay extremely high gas fees to process their transactions. This vulnerability can be exploited in a similar way for both contracts.

This can be mitigated to extent by the minDepositAmount variable, but that will just make the attack vector a bit more expensive for the attacker and the vulnerability would still be there.

To apply that to real world, assume the following scenario:

  1. Alice queues 10000 deposits for 1 wei
  2. Bob queues 1 deposit for 1e18

Since there's no way to force Alice to process her deposits, Bob can either wait for somebody else to process them or process them himself. However, whoever does that will have to pay enormous amount of gas fees.

Here's a PoC using Foundry, making use of LiquidityPoolTest test suite. However, it would work pretty much in the same way for KangarooVault, where the only prerequisite would be that there have to be currently active positions.

  function testDepositFee() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        susd.mint(alice, 1e18);
        susd.mint(bob, 1e18);

        vm.startPrank(alice);
        susd.approve(address(pool), 1e18);

        for(uint256 i = 0; i < 100000; i++) {
            pool.queueDeposit(1 wei, alice);
        }
        vm.stopPrank();

        vm.startPrank(bob);
        susd.approve(address(pool), 1e18);
        pool.queueDeposit(1e18, address(bob));

        vm.warp(block.timestamp + pool.minDepositDelay());

        pool.processDeposits(100001);

        vm.stopPrank();
  }

Running the test using the following command forge t --match-test testDepositFee -vv --gas-report yiels the following result.

The same scenario can happen when processing withdrawals. If we expand the example to extreme values (100,000,000 deposits), this would mean that the approximate gas to be paid for users that want to withdraw their funds equal approximately 2448749420000, which converted to today's ETH:USD ratio is around 4070335.77 USD.

Tools Used

Manual review, foundry.

Recommended Mitigation Steps

My recommendations for this vulnerability are the following:

  1. Replace the sequential processing of deposits and withdrawals with functionality where users can execute their own deposits and withdrawals without having to process all the deposits and withdrawals before theirs.
  2. If you insist keeping the sequential processing, add a mechanism to cancel deposits but still implement the second part of point 1.
c4-judge commented 1 year ago

JustDravee marked the issue as duplicate of #122

JustDravee commented 1 year ago

Due to the high quality of the report and the details given (particularly, the real impact on the user's funds), I'll be selecting this for the report. While High Severity can be defended here, I believe this to be more of a Griefing attack (Medium Severity, no profit motive for an attacker, but damage done to the users or the protocol) The user's assets in the protocol aren't at direct risk, even if they need to pay more Gas (on Optimism). On Immunefi, "Theft of gas" or "Unbounded gas consumption" are considered Medium Severity issues, and some projects even put "Loss of gas costs or funds will not be considered ‘loss of funds’" in their OOS section (like Olympus). Hence the Medium Severity.

c4-judge commented 1 year ago

JustDravee marked the issue as selected for report

c4-judge commented 1 year ago

JustDravee changed the severity to 2 (Med Risk)