Polynomial Protocol contest - auditor0517's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L424-L426 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L436-L447

Vulnerability details

Impact

The owner of the KangarooVault can't receive collateral from EXCHANGE when he wants to remove collateral from the vault.

Proof of Concept

KangarooVault.removeCollateral doesn't remove collateral from the EXCHANGE. KangarooVault interacts with EXCHANGE, and the owner of KangarooVault can add/remove collateral to manage short positions. In the implementation of addCollateral, it calls addCollateral of EXCHANGE as expected.

    function addCollateral(uint256 additionalCollateral) external requiresAuth nonReentrant {
        SUSD.safeApprove(address(EXCHANGE), additionalCollateral);
        EXCHANGE.addCollateral(positionData.positionId, additionalCollateral);

But in the implementation of removeCollateral, there is no such thing. KangarooVault.removeCollateral doesn't interact with EXCHANGE and only updates states.

    function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
        (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
        uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
        minColl = minColl.mulWadDown(collRatio);

        require(positionData.totalCollateral >= minColl + collateralToRemove);

        usedFunds -= collateralToRemove;
        positionData.totalCollateral -= collateralToRemove;

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

As a result, the owner of the KangarooVault can't receive collateral from EXCHANGE when he wants to remove collateral from the vault.

Tools Used

Manual Review

Recommended Mitigation Steps

We should call EXCHANGE.removeCollateral to make sure to receive collaterals.

    EXCHANGE.removeCollateral(positionData.positionId, collateralToRemove);

Lines of code

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

Vulnerability details

Impact

totalFunds is not updated in Liquidity.openShort so totalFunds will be wrong.

Proof of Concept

Liquidity.openShort updates usedFunds only and doesn't update totalFunds. totalFunds should be updated after openShort.

Tools Used

Manual Review

Recommended Mitigation Steps

totalFunds should be updated in openShort.

    totalFunds += feesCollected - externalFee;

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L472-L484 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L807-L808 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L549 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L516

Vulnerability details

Impact

usedFunds is wrong in LiquidityPool, and usedFunds tracks spent quote tokens. usedFunds is an important state in LiquidityPool, so the impact will be high.

Proof of Concept

Liquidity.closeLong and openShort don't update the state usedFunds correctly.

In the implementation of closeLong, tradeCost is added to usedFunds.

    usedFunds += int256(tradeCost);

But tradeCost already contains hedgingFees and hedgingFees are added to usedFunds in _hedge method before.

        uint256 marginRequired = _calculateMargin(hedgingSize) + hedgingFees;
        usedFunds += int256(marginRequired);

So hedgingFees are added to usedFunds twice, and usedFunds will be wrong. There are similar things in openShort method, too. In the implementation of openShort, hedgingFees are added to usedFunds twice from direct addition and _hedge method similarly.

    usedFunds += int256(totalCost + hedgingFees + externalFee); 

Tools Used

Manual Review

Recommended Mitigation Steps

we can use totalCost instead of tradeCost to update usedFunds as follows for closeLong. And same thing for openShort.

    usedFunds += int256(totalCost);

And this is for closeShort:

    usedFunds -= int256(tradeCost);

Lines of code

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

Vulnerability details

Impact

getTokenPrice can revert and the pool's deposit and withdraw will not work.

Proof of Concept

getTokenPrice returns 1e18 when totalFunds = 0, but the correct condition is totalSupply = 0.

getTokenPrice reverts when totalSupply = 0 at the following line.

    return totalValue.divWadDown(totalSupply);

If totalFunds is always 0 when totalSupply = 0, getTokenPrice will return the correct value of 1e18.

        if (totalFunds == 0) {
            return 1e18;
        }

But totalFunds can be a dust value when totalSupply = 0. This situation can be happen in several conditions, and the correct token price is 1e18 instead of reversion. If getTokenPrice reverts, there is no way to deposit and it can result a DOS.

One of the possible senarios is for the first depositor. The first depositor can deposit and withdraw in the same transaction. He will send totalFunds for the first deposit, and the token price will be totalFunds.divWadDown(totalSupply) after that. If he withdraws all liquidity tokens (= total supply), susdToReturn = tokens.mulWadDown(tokenPrice) in withdraw method.

So if totalSupply > 1e18, susdToReturn can be slightly less than totalFunds, and totalFunds can be a non-zero dust value. After this, there is no way to deposit/withdraw and this will result DOS.

Similar thing can be happen in KangarooVault because implementation of getTokenPrice is very similar.

Tools Used

Manual Review

Recommended Mitigation Steps

getTokenPrice should return 1e18 when totalSupply = 0.

[L-01] returnedAmount is wrong for delayed withdrawal in KangarooVault and LiquidityPool

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

    current.returnedAmount = availableFunds;

In KangarooVault.processWithdrawalQueue, when funds are not enough to return, returnedAmount is set to availableFunds. But the queued withdrawal will be processed in the future again, so the update is not correct.

The following is the correct implementation.

    current.returnedAmount += availableFunds;

returnedAmount is wrong, but it is not used in the codebase, the impact is low.

[L-02] depositFee should be validated in LiquidityPool

• https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L657-L661
• https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L184-L187

There is no validation about depositFee in LiquidityPool.setFees. But if depositFee > 1e18, deposit will revert and won't work.

        uint256 fees = amount.mulWadDown(depositFee);
        uint256 amountForTokens = amount - fees;

[L-03] LiquidityPool.processDeposits reverts when count = 0 and queuedDepositHead = 0

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

    assert(queuedDepositHead + count - 1 < nextQueuedDepositId);

LiquidityPool.processDeposits reverts when count = 0 and queuedDepositHead = 0 due to underflow, while it should work without reversion.

[L-04] usedFunds is not validated in LiquidityPool.hedgePositions

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

    usedFunds += marginDelta;

In hedgePositions, marginDelta can be positive so usedFunds can be increased. In the case, usedFunds should be validated that usedFunds <= totalFunds.

[L-05] VaultToken.setVault can be front-run

• https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/VaultToken.sol#L35-L40

VaultToken.setVault don't have any permission to set vault and only accepts the first call. So if admin don't set the vault right after it is constructed in the same block, an attacker can front-run and call this function to cause a DOS attack.

[L-06] maxDepositAmount is not used in KangarooVault

• https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L506-L509

The storage variable maxDepositAmount is only set and not used in KangarooVault.

[L-07] Solmate's safeApprove don't check allowance.

• https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L636-L637

        SUSD.safeApprove(address(LIQUIDITY_POOL), maxCost);
        uint256 totalCost = EXCHANGE.closeTrade(tradeParams);

In KangarooVault._clearPendingOpenOrders, it approves maxCost, but only totalCost is consumed in LIQUIDITY_POOL. totalCost can be less than maxCost. So if safeApprove is from OpenZeppelin, _clearPendingOpenOrders will work only once because OpenZeppelin's safeApprove checks allowance. But solmate's safeApprove doesn't check allowance and there is no problem in _clearPendingOpenOrders at the moment. If there is a plan to replace solmate's SafeTransferLib by OpenZeppelin's, this problem should be considered.