Polynomial Protocol contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

If a user debt is higher than his overall collateral balance then user will never be liquidated as liquidator would be at loss (liquidatee does not have required balanace to pay liquidator)

Proof of Concept

1. Assume User A safetyRatio has fallen below WIPEOUT_CUTOFF. This means the position can be liquidated by any other user

2. User B sees the opportunity and liquidate User A

3. This calls below liquidate function

function liquidate(uint256 positionId, uint256 debt, address user)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCollateralReturned)
    {
...
uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice);
        uint256 liqBonus = collateralClaim.mulWadDown(coll.liqBonus);
        totalCollateralReturned = liqBonus + collateralClaim;
        if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

        ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned);
...
}

4. Let's say for debt X, the collateralClaim came out to be Y. If coll.liqBonus was Z then overall collateral required would be YZ for X debt

5. Now if userCollateral.amount is YZ-10 then overall collateral required would be reduced from YZ to YZ-10

if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

6. This means liquidator will be paid off YZ-10 when he was expecting YZ, bringing him to an overall loss of 10

ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned);

7. If Liquidator was cautious enough to not liquidate such positions then User A will not be liquidated by anyone and debt will not be cleared up

Recommended Mitigation Steps

Ensure that liquidator is always in benefit on liquidation event.

Lines of code

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

Vulnerability details

Impact

If an approved collateral has later started say taking fees on transfer then protocol has no way to remove such collateral. The current deposit logic cannot handle fee on transfer token and would give more funds to user then actually obtained by contract

Proof of Concept

1. Assume protocol was supporting collateral X (say USDT which has fee currently set as 0)
2. After some time collateral introduces fee on transfer
3. Protocol does not have a way to remove a whitelisted collateral
4. Problem begins once user starts depositing such collateral

function _addCollateral(uint256 positionId, uint256 amount) internal {
...
ERC20(shortPosition.collateral).safeTransferFrom(msg.sender, address(this), amount);
        ERC20(shortPosition.collateral).safeApprove(address(shortCollateral), amount);

        shortToken.adjustPosition(
            positionId,
            msg.sender,
            shortPosition.collateral,
            shortPosition.shortAmount,
            shortPosition.collateralAmount + amount
        );
        shortCollateral.collectCollateral(shortPosition.collateral, positionId, amount);
...
}

5. In this case amount is transferred from user to contract but contract will only receive amount-fees. But contract will still adjust position with full amount instead of amount-fees which is incorrect

Recommended Mitigation Steps

Add a way to disapprove collateral so that if in future some policy changes for a particular collateral, protocol can stop supporting it. This will it would only have to deal with existing collateral which can be wiped out slowly using public announcement