Polynomial Protocol contest - adriro's results

Lines of code

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

Vulnerability details

Impact

During a liquidation, the system calculates the amount of collateral that should be sent to the liquidator based on the amount of debt being repaid. This is calculated in the liquidate function present in the ShortCollateral contract:

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

function liquidate(uint256 positionId, uint256 debt, address user)
    external
    override
    onlyExchange
    nonReentrant
    returns (uint256 totalCollateralReturned)
{
    UserCollateral storage userCollateral = userCollaterals[positionId];

    bytes32 currencyKey = synthetixAdapter.getCurrencyKey(userCollateral.collateral);
    Collateral memory coll = collaterals[currencyKey];

    (uint256 markPrice,) = exchange.getMarkPrice();
    (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);
    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);

    emit LiquidateCollateral(positionId, user, userCollateral.collateral, debt, totalCollateralReturned);
}

As we can see, the calculated collateral amount (totalCollateralReturned variable) may be greater than the available collateral amount in the position, in which case it will be capped to that value. This is expected, as volatility or extreme market conditions may render the collateral insufficient to cover the debt. However, the liquidation process in the Exchange contract doesn't provide any kind of safeguard for the user to cover against these scenarios in which the expected collateral the liquidator will get differs from the moment the transaction is sent until the transaction is effectively processed by the blockchain.

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

function _liquidate(uint256 positionId, uint256 debtRepaying) 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);

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

Proof of Concept

As an easy example, let's assume an unstable market and a position that can be liquidated.

1. Liquidator sends transaction to call Exchange.liquidate.
2. Collateral price tanks and falls short to cover debt.
3. Transaction is executed, liquidator gets less than expected.

Recommendation

Add a minCollateralReturned parameter to the liquidate function of the Exchange contract. Revert the transaction if the calculated collateral to be returned is below this minimum.

+ function liquidate(uint256 positionId, uint256 debtRepaying, uint256 minCollateralReturned)
      external
      override
      nonReentrant
      whenNotPaused("EXCHANGE_LIQUIDATE")
  {
+     _liquidate(positionId, debtRepaying, minCollateralReturned);
      _updateFundingRate();
  }

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

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

Report

• Non Critical Issues (2)
• Low Issues (8)

Non Critical Issues

<a name="NC-1"></a>[NC-1] Bool expression compared to literal value

Bool expressions do not need to be compared against a literal value. For example, aBoolExpression == true can be directly stated as aBoolExpression, or aBoolExpression == false as !aBoolExpression.

Instances (1):

File: src/utils/PauseModifier.sol

12:         require(systemManager.isPaused(key) == false);

<a name="NC-2"></a>[NC-2] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18

Instances (8):

File: src/KangarooVault.sol

151:     mapping(uint256 => QueuedDeposit) public depositQueue;

154:     mapping(uint256 => QueuedWithdraw) public withdrawalQueue;

File: src/LiquidityPool.sol

145:     mapping(uint256 => QueuedDeposit) public depositQueue;

148:     mapping(uint256 => QueuedWithdraw) public withdrawalQueue;

File: src/ShortCollateral.sol

34:     mapping(bytes32 => Collateral) public collaterals;

37:     mapping(uint256 => UserCollateral) public userCollaterals;

File: src/ShortToken.sol

26:     mapping(uint256 => ShortPosition) public shortPositions;

File: src/SystemManager.sol

50:     mapping(bytes32 => bool) public isPaused;

Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (10):

File: src/Exchange.sol

2: pragma solidity ^0.8.9;

File: src/KangarooVault.sol

2: pragma solidity ^0.8.9;

File: src/LiquidityPool.sol

2: pragma solidity ^0.8.9;

File: src/LiquidityToken.sol

2: pragma solidity ^0.8.9;

File: src/PowerPerp.sol

2: pragma solidity ^0.8.9;

File: src/ShortCollateral.sol

2: pragma solidity ^0.8.9;

File: src/ShortToken.sol

2: pragma solidity ^0.8.9;

File: src/SynthetixAdapter.sol

2: pragma solidity ^0.8.9;

File: src/SystemManager.sol

2: pragma solidity ^0.8.9;

File: src/utils/PauseModifier.sol

3: pragma solidity ^0.8.9;

<a name="L-2"></a>[L-2] KangarooVault should initialize systemManager

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

KangarooVault contract inherits from PauseModifier but doesn't initialize the systemManager storage variable. The contract should initialize this variable or remove the PauseModifier base contract as the pause modifier isn't used.

<a name="L-3"></a>[L-3] LiquidityPool withdraw should use safeTransfer

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

The SUSD transfer on line 254 doesn't use the safe wrapper as opposed to all other calls in the contract.

<a name="L-4"></a>[L-4] LiquidityPool processWithdraws overwrites returnedAmount for withdrawals processed in multiple steps

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

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

The returnedAmount field in the QueuedWithdraw struct is incorrectly overwritten when the withdraw is processed and available funds aren't enough to cover the withdrawal. As these cases are processed in multiple steps, the implementation should add the amounts instead of overwriting the value for returnedAmount.

  if (susdToReturn > availableFunds) {
+     current.returnedAmount += availableFunds;
      uint256 tokensBurned = availableFunds.divWadUp(tokenPrice);
      totalQueuedWithdrawals -= tokensBurned;
      current.withdrawnTokens -= tokensBurned;
      totalFunds -= availableFunds;
      SUSD.safeTransfer(current.user, availableFunds);

      emit ProcessWithdrawalPartially(
          current.id, current.user, tokensBurned, availableFunds, current.requestedTime
          );

      return;
  } else {
      // Complete full withdrawal
+     current.returnedAmount += susdToReturn;
      totalQueuedWithdrawals -= current.withdrawnTokens;
      totalFunds -= susdToReturn;
      SUSD.safeTransfer(current.user, susdToReturn);

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

      current.withdrawnTokens = 0;
  }

<a name="L-5"></a>[L-5] Missing event for important parameter change

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Instances (1):

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

<a name="L-6"></a>[L-6] SystemManager initialization can be front-runned

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SystemManager.sol#L62-L82

The init function present in the SystemManager contract can be front-runned during the contracts bootstrapping process. Consider adding the requiresAuth modifier to prevent anyone from calling this function.

  function init(
      address _pool,
      address _powerPerp,
      address _exchange,
      address _liquidityToken,
      address _shortToken,
      address _synthetixAdapter,
      address _shortCollateral
+ ) public requiresAuth {
      require(!isInitialized);
      refreshSynthetixAddresses();

      pool = ILiquidityPool(_pool);
      powerPerp = IPowerPerp(_powerPerp);
      exchange = IExchange(_exchange);
      liquidityToken = ILiquidityToken(_liquidityToken);
      shortToken = IShortToken(_shortToken);
      synthetixAdapter = ISynthetixAdapter(_synthetixAdapter);
      shortCollateral = IShortCollateral(_shortCollateral);
      isInitialized = true;
  }

<a name="L-7"></a>[L-7] Synthetix market can be empty in refreshSynthetixAddresses

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SystemManager.sol#L84-L87

The refreshSynthetixAddresses function present in the SystemManager contract refreshes the market address using the marketForKey function of the Synthetix FuturesMarketManager contract. As markets can be removed, special care must be taken if this happens and the return value is empty.

<a name="L-8"></a>[L-8] VaultToken setVault initialization can be front-runned

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

The function setVault present in the VaultToken contract can be front-runned during the Vault initialization process, which will give the caller minting capabilities over the token.

The VaultToken can be created directly by the KangarooVault during construction time. For example, remove setVault and set the vault in the VaultToken constructor:

constructor(string memory name, string memory symbol) ERC20(name, symbol, 18) {
  vault = msg.sender;
}

Then, KangarooVault can create the VaultToken during its own constructor:

constructor(
    ERC20 _susd,
    IExchange _exchange,
    ILiquidityPool _pool,
    IPerpsV2Market _perpMarket,
    bytes32 _underlyingKey,
    bytes32 _name
) Auth(msg.sender, Authority(address(0x0))) {
    VAULT_TOKEN = new VaultToken(...);
    EXCHANGE = _exchange;
    LIQUIDITY_POOL = _pool;
    PERP_MARKET = _perpMarket;
    UNDERLYING_SYNTH_KEY = _underlyingKey;
    name = _name;
    SUSD = _susd;
}