Polynomial Protocol contest - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

The protocol does not adequately increase the calculated fees to the totalFunds storage variable in the open short operations consequently the liquidity providers receive less yields and the protocol receive less funds. Those calculated fees are lost.

Proof of Concept

The fees are calculated in the 511 code line but those are not increased to the totalFunds storage variable:

File: LiquidityPool.sol
491:     /// @notice Called by exchange when a new short position is created
492:     /// @param amount Amount of square perp being shorted
493:     /// @param user Address of the user
494:     function openShort(uint256 amount, address user, bytes32 referralCode)
495:         external
496:         override
497:         onlyExchange
498:         nonReentrant
499:         returns (uint256 totalCost)
500:     {
501:         (uint256 markPrice, bool isInvalid) = getMarkPrice();
502:         require(!isInvalid);
503: 
504:         uint256 tradeCost = amount.mulWadDown(markPrice);
505:         uint256 fees = orderFee(-int256(amount));
506:         totalCost = tradeCost - fees;
507: 
508:         SUSD.safeTransfer(user, totalCost);
509: 
510:         uint256 hedgingFees = _hedge(-int256(amount), false);
511:         uint256 feesCollected = fees - hedgingFees;
512:         uint256 externalFee = feesCollected.mulWadDown(devFee);
513: 
514:         SUSD.safeTransfer(feeReceipient, externalFee);
515: 
516:         usedFunds += int256(totalCost + hedgingFees + externalFee);
517:         require(usedFunds <= 0 || totalFunds >= uint256(usedFunds));
518: 
519:         emit RegisterTrade(referralCode, feesCollected, externalFee);
520:         emit OpenShort(markPrice, amount, fees);
521:     }

Tools used

VScode

Recommended Mitigation Steps

Increment the feesCollected to the totalFunds storage variable in the openShort() function.

Lines of code

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

Vulnerability details

Impact

The LiquidityPool.sol::queueDeposit() function helps to queue the user deposit and then the deposit will be processed by the LiquidityPool.sol::processDeposits() function after the delay is completed. The deposits are used to cover the margin required in every operation.

The problem is that an attacker can create many deposits with zero amounts, so it is possible to flood the depositQueue with zero amount deposits and the processDeposits() function will cost more work/gas to process.

Please see the next scenario:

1. For some reason there are not sUSD in the LiquidityPool.
2. The protocol needs to cover his margins using the increaseMargin() function but is not possible because there are not any sUSD in the LiquidityPool.
3. The liquidity providers use queueDeposit() function because the function doesn't have fees.
4. The attacker frontruns the liquidity providers and he floods the depositQueue with zero amounts causing the processing of the processDeposits() function to cost more work/gas.
5. The protocol can't hedge his exposure and they need to use the deposit() function instead.

At the end, the attacker can cause the processDeposits() function to take more work/gas to process, causing valid deposits to be unavailable.

Proof of Concept

I created a basic test where you can see that it is possible to queue deposits with amount zero:

function testZeroQueueDeposit() public {
    // It is possible to flood the depositQueue with zero amounts in the queueDeposit()
    // causing a DOS in the processDeposits()
    // 1. Call the queueDeposit() with zero amounts
    // 2. Assert queuedDepositHead and nextQueuedDepositId
    // 3. Call the processDeposits()
    // 4. Assert the incremention of queuedDepositHead
    //
    // 1. Call the queueDeposit() with zero amounts
    //
    pool.queueDeposit(0, user_1);
    pool.queueDeposit(0, user_2);
    pool.queueDeposit(0, user_3);
    //
    // 2. Assert queuedDepositHead and nextQueuedDepositId
    //
    assertEq(pool.queuedDepositHead(), 1);
    assertEq(pool.nextQueuedDepositId(), 4);
    //
    // 3. Call the processDeposits()
    //
    vm.warp(block.timestamp + pool.minDepositDelay());
    pool.processDeposits(1);
    //
    // 4. Assert the incremention of queuedDepositHead
    //
    assertEq(pool.queuedDepositHead(), 2);
}

Tools used

VScode

Recommended Mitigation Steps

Checks that the deposited amount is not zero in the queueDeposit() function.

Lines of code

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

Vulnerability details

Impact

The queueWithdraw() function helps to the user to queue his withdrawals and the processWithdraws() function helps to process all the users withdrawals.

The problem here is that the attacker can flood the withdrawalQueue with zero amounts, causing the processWithdraws() cost more gas to process the user withdrawals. The same behaivour is happening in the KangarooVault.sol::initiateWithdrawal().

The withdrawals can be delayed by the attacker which generates mistrust in the protocol to the users.

Proof of Concept

I created a test in LiquidityPool.Deposits.t.sol where it is possible to flood the withdrawalQueue with zero amount via the queueWithdraw() function.

function testZeroQueueWithdraw() public {
    // It is possible to flood the withdrawalQueue with zero amounts in the queueWithdraw()
    // causing a DOS in the processWithdraws()
    //
    // 1. Deposit some money in order to have totalFunds
    //
    susd.mint(user_2, 1e20);
    vm.startPrank(user_2);
    susd.approve(address(pool), 1e20);
    pool.deposit(69e18, user_2);
    vm.stopPrank();
    //
    // 2. Call queueWithdraw with zero withdraw amount
    //
    pool.queueWithdraw(0, user_1);
    pool.queueWithdraw(0, user_2);
    pool.queueWithdraw(0, user_3);
    pool.queueWithdraw(0, address(1337));
    //
    // 3. Assert queuedWithdrawalHead() and nextQueuedWithdrawalId()
    //
    assertEq(pool.queuedWithdrawalHead(), 1);
    assertEq(pool.nextQueuedWithdrawalId(), 5);
    //
    // 4. Call processWithdraws()
    //
    vm.warp(block.timestamp + pool.minWithdrawDelay());
    pool.processWithdraws(1);
    //
    // 5. Assert the increase of queuedWithdrawalHead()
    //
    assertEq(pool.queuedWithdrawalHead(), 2);
}

Tools used

VScode

Recommended Mitigation Steps

Check the zero amount value in the LiquidityPool.sol::queueWithdraw() and KangarooVault.sol::initiateWithdrawal() functions.