Polynomial Protocol contest - 0xRobocop's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortToken.sol#L82 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L388

Vulnerability details

Impact

When closing completely a short position, if the collateralAmount passed as parameter is smaller than the collateralAmount stored on shortPositions[positionId].collateralAmount then the difference will get stucked on the ShortCollateral.sol contract.

This happens because the adjustPosition function on the ShortToken.sol contract burns the ERC721 token represented by positionId when the short position is closed completely without making sure that all the collateral have been removed at the same time.

The removeCollateral function will fail since the ERC721 token represented by positionId has been burned.

Proof of Concept

Add the next function on the Exchange.Simple.t.sol file and run:

forge test --match-contract SimpleExchangeTest --match-test testShortCloseTradeCollateralStuck

function testShortCloseTradeCollateralStuck() public {
        uint256 pricingConstant = exchange.PRICING_CONSTANT();
        uint256 expectedPrice = initialPrice.mulDivDown(initialPrice, pricingConstant);
        uint256 collateralAmount = expectedPrice * 2; // 200% Collateral ratio

        (uint256 positionId,) = openShort(1e18, collateralAmount, user_1);

        uint256 collateralToGetBack = collateralAmount / 2;

        // user_1 is about to close the short position completely
        // but instead of passing collateralAmount as parameter, he 
        // passed collateralToGetBack
        closeShort(positionId, 1e18, 1000e18, collateralToGetBack, user_1);

        (address _collateral, uint256 _collateralAmount) = shortCollateral.userCollaterals(positionId);

        // The difference of collateral is still on the ShortCollteral.sol contract
        assertEq(_collateralAmount, collateralToGetBack);
        assertEq(shortToken.balanceOf(user_1), 0);
        
        // Remove Collateral fails.
        vm.prank(user_1);
        vm.expectRevert();
        exchange.removeCollateral(positionId, collateralToGetBack);
        vm.stopPrank();
}

Tools Used

Manual review

Recommended Mitigation Steps

When a short position is about to be closed completely make sure that all the collateral is removed at the same time. For example on the adjustPosition function add:

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

if (position.shortAmount == 0) {
   require(position.collateralAmount == 0);
   _burn(positionId);
}

Or for better UX, make the Exchange.sol contract to send all the collateral when a short is about to be closed completely.

Lines of code

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

Vulnerability details

Impact

Liquidity Providers on the LiquidityPool.sol contract expect to get a gain via fees paid by traders when they open/close longs or shorts. This is done by increasing the totalFunds variable by feesCollected - externalFee.

The openShort function lacks this increase, even though a fee is deducted from the sUSD amount to send to the user, also feeRecipient is sent his share of the fee. This will make liquidity providers to not earn fees when someone open a short position.

Tools Used

Manual Review

Recommended Mitigation Steps

Add totalFunds += feesCollected - externalFee;