Polynomial Protocol contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

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

    } 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) {
            _burn(positionId); <- @audit-issue burns the token even if it still has collateral
        }

When adjusting a position, the position token is burned if shortAmount == 0. The issue with this is that it doesn't check if the token has any collateral still associated with it. The result is that if there is still collateral assigned to the token and it is burned then all collateral assigned to that token will not longer be accessible and will be permanently lost.

Tools Used

Manual Review

Recommended Mitigation Steps

Only burn the position if BOTH collateral and shortAmount = 0

Lines of code

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

Vulnerability details

Impact

Collateral is permanently trapped inside short position

Proof of Concept

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

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

KangarooVault#removeCollateral never actually makes a call to the exchange to remove the requested collateral. The result is that all collateral added to the short position is permanently stuck there.

Tools Used

Manual Review

Recommended Mitigation Steps

Make a call to exchange to remove the collateral:

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