Polynomial Protocol contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortToken.sol#L82-L84

Vulnerability details

Impact

adjustPosition() when burn NFT , lack of judgment on whether collateral is still available, which may result in loss of collateral

Proof of Concept

When Exchange.sol needs to adjust the position, shortToken.adjustPosition() will be called The code to implement adjustPosition() is as follows:

    function adjustPosition(
        uint256 positionId,
        address trader,
        address collateral,
        uint256 shortAmount,
        uint256 collateralAmount
    ) external onlyExchange returns (uint256) {
        if (positionId == 0) {
....
        } else {
            require(trader == ownerOf(positionId));

            ShortPosition storage position = shortPositions[positionId];

            if (shortAmount >= position.shortAmount) {
                totalShorts += shortAmount - position.shortAmount;
            } else {
                totalShorts -= position.shortAmount - shortAmount;
            }

            position.collateralAmount = collateralAmount;
            position.shortAmount = shortAmount;

            if (position.shortAmount == 0) {  //<--------There is no check if collateralAmount is 0
                _burn(positionId);
            }
        }

The current implementation, if shortAmount is adjusted to 0, puts burn NFT Here is a problem, only check the shortAmount is 0, and not check whether the position.collateralAmount is 0 so there may be still have collateral on the NFT, but the NFT is destroyed, the user can not retrieve collateral

Example: Exchange.sol#closeTrade will call this method, and the user can specify the reduced shortAmount and collateralAmount The user adjusts the shortAmount to 0, and wants to retain the collateral, but the current implementation , the entire NFT will be burned, the collateral will be lost

    function _closeTrade(TradeParams memory params) internal returns (uint256) {
        if (params.isLong) {
...
        } else {
...
            uint256 totalShortAmount = shortPosition.shortAmount - params.amount;
            uint256 totalCollateralAmount = shortPosition.collateralAmount - params.collateralAmount;
....
            shortToken.adjustPosition(   //<------if totalShortAmount =0 ,but totalCollateralAmount>0 ,NFT Will still be burned
                params.positionId, msg.sender, params.collateral, totalShortAmount, totalCollateralAmount
            );                            

Example: alice has shortToken: positionId=1 shortAmount=10 collateralAmount=100

alice executes closeTrade: positionId=1 TradeParams.Amount=10 params.collateralAmount=0 //retains collateral

This will cause NFT to be burned and collateral will be lost 100

Tools Used

Recommended Mitigation Steps

    function adjustPosition(
        uint256 positionId,
        address trader,
        address collateral,
        uint256 shortAmount,
        uint256 collateralAmount
    ) external onlyExchange returns (uint256) {
        if (positionId == 0) {
....
        } else {
            require(trader == ownerOf(positionId));

            ShortPosition storage position = shortPositions[positionId];

            if (shortAmount >= position.shortAmount) {
                totalShorts += shortAmount - position.shortAmount;
            } else {
                totalShorts -= position.shortAmount - shortAmount;
            }

            position.collateralAmount = collateralAmount;
            position.shortAmount = shortAmount;

-           if (position.shortAmount == 0) {
+           if (position.shortAmount == 0 && position.collateralAmount==0) {
                _burn(positionId);
            }

Lines of code

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

Vulnerability details

Impact

KangarooVault.removeCollateral() Lack of actual calls EXCHANGE.removeCollateral(), Resulting in the collateral not being retrieved

Proof of Concept

removeCollateral() is used to remove collateral The code is as follows:

    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);

        //<------no execute EXCHANGE.removeCollateral()

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

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

In the above code, forget to actually call EXCHANGE.removeCollateral()

But positionData.totalCollateral has a corresponding decrease, so that the reduced number of collateral can no longer be retrieved and is locked

Tools Used

Recommended Mitigation Steps

    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);

+       EXCHANGE.removeCollateral(positionData.positionId,collateralToRemove);

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

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

Lines of code

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

Vulnerability details

Impact

The malicious user specifies receive user=address(SUSD), thus blocking the entire withdrawal queue

Proof of Concept

processWithdraws() is used to process the user's withdrawal request Currently the processing logic is one by one in order and cannot be skipped The implementation code is as follows:

    function processWithdraws(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_WITHDRAWS") {
....

        for (uint256 i = 0; i < count; i++) {
            uint256 tokenPrice = getTokenPrice();
            QueuedWithdraw storage current = withdrawalQueue[queuedWithdrawalHead];

           if (susdToReturn > availableFunds) {
....
            } else {

                current.returnedAmount = susdToReturn;
                totalQueuedWithdrawals -= current.withdrawnTokens;
                totalFunds -= susdToReturn;

                SUSD.safeTransfer(current.user, susdToReturn); //<--------execute transfer

                emit ProcessWithdrawal(
                    current.id, current.user, current.withdrawnTokens, susdToReturn, current.requestedTime
                    );

                current.withdrawnTokens = 0;
            }  
            queuedWithdrawalHead++; //<----- ++, next turn          

There is a problem, if one user's SUSD.safeTransfer() fails, then the queue behind it will be blocked and cannot be cancelled

So SUSD.transfer() could possibly revert? Check inside SUSD.transfer() method, SUSD has a restriction on to, it can't be address(SUSD) https://etherscan.io/address/0x10A5F7D9D65bCc2734763444D4940a31b109275f#code

    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"); //<-----if to == address(this) or address(proxy) will revert

        // 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;
    }

The code above shows that if execute SUSD.transfer(to=address(SUSD)) it will revert

And we can specify the user when we join the withdrawal queue

    function queueWithdraw(uint256 tokens, address user)   //<-------can set user=address(SUSD)    

As mentioned above, if a malicious user generates a withdrawal queue with to=address(SUSD), the subsequent user's withdrawals will all be blocked and the funds in the queue will be lost

Note: KangarooVault.processWithdrawalQueue() also has the same problem

Tools Used

Recommended Mitigation Steps

queueWithdraw() cancel parameter user

-   function queueWithdraw(uint256 tokens, address user)
+   function queueWithdraw(uint256 tokens)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_WITHDRAW")
    {
        require(liquidityToken.balanceOf(msg.sender) >= tokens);
        QueuedWithdraw storage newWithdraw = withdrawalQueue[nextQueuedWithdrawalId];
        newWithdraw.id = nextQueuedWithdrawalId++;
-       newWithdraw.user = user;
+       newWithdraw.user = msg.sender;
        newWithdraw.withdrawnTokens = tokens;
        newWithdraw.requestedTime = block.timestamp;
        totalQueuedWithdrawals += tokens;
        liquidityToken.burn(msg.sender, tokens);

        emit InitiateWithdrawal(newWithdraw.id, msg.sender, user, tokens);
    }

Lines of code

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

Vulnerability details

Impact

_liquidate () Lack of slippage protection，The amount of collateral is lower than user expectations

Proof of Concept

_liquidate () code is as follows:

    function _liquidate(uint256 positionId, uint256 debtRepaying) internal {//<------without minCollateralReturned
        uint256 maxDebtRepayment = shortCollateral.maxLiquidatableDebt(positionId);
        require(maxDebtRepayment > 0);
        if (debtRepaying > maxDebtRepayment) debtRepaying = maxDebtRepayment;

        IShortToken.ShortPosition memory position = shortToken.shortPositions(positionId);

        uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender);

        address user = shortToken.ownerOf(positionId);

        uint256 finalPosition = position.shortAmount - debtRepaying;
        uint256 finalCollateralAmount = position.collateralAmount - totalCollateralReturned;

        shortToken.adjustPosition(positionId, user, position.collateral, finalPosition, finalCollateralAmount);

        pool.liquidate(debtRepaying);
        
        powerPerp.burn(msg.sender, debtRepaying);

        emit Liquidate(user, positionId, msg.sender, debtRepaying, position.collateral, totalCollateralReturned);
    }

Since markPrice and collateralPrice are real-time prices, they may be different from the user's original expectation, resulting in slippage in the resulting collateral. Need to add minCollateralReturned to protect

Tools Used

Recommended Mitigation Steps

-   function _liquidate(uint256 positionId, uint256 debtRepaying) internal {
+   function _liquidate(uint256 positionId, uint256 debtRepaying ,uint256 minCollateralReturned) internal {
        uint256 maxDebtRepayment = shortCollateral.maxLiquidatableDebt(positionId);
        require(maxDebtRepayment > 0);
        if (debtRepaying > maxDebtRepayment) debtRepaying = maxDebtRepayment;

        IShortToken.ShortPosition memory position = shortToken.shortPositions(positionId);

        uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender);
+       require(totalCollateralReturned>=minCollateralReturned,"bad totalCollateralReturned");

        address user = shortToken.ownerOf(positionId);

        uint256 finalPosition = position.shortAmount - debtRepaying;
        uint256 finalCollateralAmount = position.collateralAmount - totalCollateralReturned;

        shortToken.adjustPosition(positionId, user, position.collateral, finalPosition, finalCollateralAmount);

        pool.liquidate(debtRepaying);
        
        powerPerp.burn(msg.sender, debtRepaying);

        emit Liquidate(user, positionId, msg.sender, debtRepaying, position.collateral, totalCollateralReturned);
    }

L-01:Exchange._addCollateral() Missing to determine whether the current collateral is still valid, _removeCollateral() and _openTrade() both have checks, it is recommended to also add checks to avoid the collateral is no longer valid

   function _addCollateral(uint256 positionId, uint256 amount) internal {
..

        shortToken.adjustPosition(
            positionId,
            msg.sender,
            shortPosition.collateral,
            shortPosition.shortAmount,
            shortPosition.collateralAmount + amount
        );
+       //for check collateral valid
+       shortCollateral.getMinCollateral(shortPosition.shortAmount, shortPosition.collateralAmount + amount);
        shortCollateral.collectCollateral(shortPosition.collateral, positionId, amount);

        emit AddCollateral(msg.sender, positionId, shortPosition.collateral, amount);
    }

L-02:processDepositQueue() Each QueuedDeposit re-fetches the price more realistically, and each QueuedDeposit may modify the TokenPrice like processWithdrawalQueue():

    function processDepositQueue(uint256 idCount) external nonReentrant {
-       uint256 tokenPrice = getTokenPrice();

        for (uint256 i = 0; i < idCount; i++) {
+           uint256 tokenPrice = getTokenPrice();        
            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;
            VAULT_TOKEN.mint(current.user, tokensToMint);

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

            current.depositedAmount = 0;
            queuedDepositHead++;
        }
    }

L-03 LiquidityPool.minDepositDelay The limit must be greater than 0, to avoid QueuedDeposit and deposit() can be the same as real-time, but do not have to pay depositFee

    function setMinDelays(uint256 _minDepositDelay, uint256 _minWithdrawDelay) external requiresAuth {
+       require(_minDepositDelay>0 && _minWithdrawDelay>0,">0");
        emit UpdateDelays(minDepositDelay, _minDepositDelay, minWithdrawDelay, _minWithdrawDelay);
        minDepositDelay = _minDepositDelay;
        minWithdrawDelay = _minWithdrawDelay;
    }

L-04:EXCHANGE.sol#_addCollateral() Remove the restriction on determining whether the MarkPrice is valid or not, because to avoid liquidation, the user should be able to add collateral regardless of the current MarkPrice being temporarily invalid.

    function _addCollateral(uint256 positionId, uint256 amount) internal {
        require(positionId != 0);
-       (, bool isInvalid) = getMarkPrice();
-       require(!isInvalid);