Polynomial Protocol contest - joestakey's results

Lines of code

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

Vulnerability details

The admin can call KangarooVault.addCollateral to add additional collateral to a Power Perp position.

File: src/KangarooVault.sol
424:     function addCollateral(uint256 additionalCollateral) external requiresAuth nonReentrant {
425:         SUSD.safeApprove(address(EXCHANGE), additionalCollateral);
426:         EXCHANGE.addCollateral(positionData.positionId, additionalCollateral);
427: 
428:         usedFunds += additionalCollateral;
429:         positionData.totalCollateral += additionalCollateral;
430: 
431:         emit AddCollateral(positionData.positionId, additionalCollateral);
432:     }

This transfers SUSD to the EXCHANGE and updates the usedFunds and positionData.totalCollateral

The function KangarooVault.removeCollateral allows the admin to remove collateral if a position is healthy enough

File: src/KangarooVault.sol
436:     function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
437:         (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
438:     uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
439:         minColl = minColl.mulWadDown(collRatio);
440: 
441:         require(positionData.totalCollateral >= minColl + collateralToRemove);
442:         
443:         usedFunds -= collateralToRemove;
444:         positionData.totalCollateral -= collateralToRemove;
445: 
446:         emit RemoveCollateral(positionData.positionId, collateralToRemove);
447:     }    

The issue is that this function does not call EXCHANGE.removeCollateral. While it updates storage, it does not actually retrieve any collateral.

Impact

2 problems arising:

• processWithdrawalQueue will revert unexpectedly, as usedFunds will be lower than it should, leading to availableFunds being greater than the real balance of SUSD available.
• the main problem: the amount of collateral "removed" in removeCollateral will be lost :

When closing a position in KangarooVault._closePosition, the amount of collateral to retrieve is written in tradeParams.collateralAmount. As you can see below, it is capped by positionData.totalCollateral, which was decremented in removeCollateral.

File: src/KangarooVault.sol
687:         if (amt >= positionData.shortAmount) {
688:             longPositionToClose = positionData.longPerp;
689: 
690:             tradeParams.amount = positionData.shortAmount;
691:             tradeParams.collateralAmount = positionData.totalCollateral; //@audit here
692:         } else {
693:             longPositionToClose = amt.mulDivDown(positionData.longPerp, positionData.shortAmount);
694:             uint256 collateralToRemove = amt.mulDivDown(positionData.totalCollateral, positionData.shortAmount);
695: 
696:             tradeParams.amount = amt;
697:             tradeParams.collateralAmount = collateralToRemove;
698:         }
699: 
700:         SUSD.safeApprove(address(LIQUIDITY_POOL), maxCost);
701:         uint256 totalCost = EXCHANGE.closeTrade(tradeParams);

This is the amount of collateral that will be transferred back to the trader (here the KangarooVault)

src/Exchange.sol _closeTrade()
316: shortCollateral.sendCollateral(params.positionId, params.collateralAmount);

File: src/ShortCollateral.sol
106:     function sendCollateral(uint256 positionId, uint256 amount) external override onlyExchange nonReentrant {
107:         UserCollateral storage userCollateral = userCollaterals[positionId];
108: 
109:         userCollateral.amount -= amount;
110: 
111:         address user = shortToken.ownerOf(positionId);
112: 
113:         ERC20(userCollateral.collateral).safeTransfer(user, amount); //@audit capped by `positionData.totalCollateral`

In conclusion, calling removeCollateral will result in that amount being lost.

Proof of Concept

Amend this test to KangarooVault.t.sol, which shows how collateral is not transferred upon calling removeCollateral()

429:     function testCollateralManagement() public {
430:         uint256 amt = 1e18;
431:         uint256 collDelta = 1000e18;
432: 
433:         kangaroo.openPosition(amt, 0);
434:         skip(100);
435:         kangaroo.executePerpOrders(emptyData);
436:         kangaroo.clearPendingOpenOrders(0);
437: 
438:         (,,,,,,, uint256 initialColl,) = kangaroo.positionData();
+439:         uint256 balanceBefore = susd.balanceOf(address(kangaroo));
440: 
441:         kangaroo.addCollateral(collDelta);
+442:         uint256 balanceAfter = susd.balanceOf(address(kangaroo));
+443:         assertEq(collDelta, balanceBefore - balanceAfter);
444:         (,,,,,,, uint256 finalColl,) = kangaroo.positionData();
445: 
446:         assertEq(finalColl, initialColl + collDelta);
447: 
+448:         uint256 balanceBefore2 = susd.balanceOf(address(kangaroo));
449:         kangaroo.removeCollateral(collDelta);
+450:         uint256 balanceAfter2 = susd.balanceOf(address(kangaroo));
+451:         assertEq(0, balanceAfter2 - balanceBefore2); //@audit collateral not removed
452: 
453:         (,,,,,,, uint256 newColl,) = kangaroo.positionData();
454: 
455:         assertEq(newColl, initialColl);
456:     }

Tools Used

Manual Analysis, Foundry

Mitigation

Ensure Exchange.removeCollateral is called:

File: src/KangarooVault.sol
436:     function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
437:         (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
438:         uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
439:         minColl = minColl.mulWadDown(collRatio);
440: 
441:         require(positionData.totalCollateral >= minColl + collateralToRemove);
442:         
443:         usedFunds -= collateralToRemove;
444:         positionData.totalCollateral -= collateralToRemove;
445: 
+            EXCHANGE.removeCollateral(positionData.positionId, collateralToRemove)
446:         emit RemoveCollateral(positionData.positionId, collateralToRemove);
447:     }    

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L210 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L235

Vulnerability details

Liquidation of a position is initiated in Exchange.liquidate. The health check is done in liquidate(), by assessing whether shortCollateral.maxLiquidatableDebt(positionId) is not zero:

File: src/Exchange.sol
334: uint256 maxDebtRepayment = shortCollateral.maxLiquidatableDebt(positionId);
335:         require(maxDebtRepayment > 0);

Looking at maxLiquidatableDebt(positionId), it uses the liquidation ratio, the collateral amount and price, the position and the mark price to determine if a position is healthy

File: src/ShortCollateral.sol
230: uint256 safetyRatioNumerator = position.collateralAmount.mulWadDown(collateralPrice);
231:         uint256 safetyRatioDenominator = position.shortAmount.mulWadDown(markPrice);
232:         safetyRatioDenominator = safetyRatioDenominator.mulWadDown(collateral.liqRatio);
233:         uint256 safetyRatio = safetyRatioNumerator.divWadDown(safetyRatioDenominator);
234: 
235:         if (safetyRatio > 1e18) return maxDebt;
236: 
237:         maxDebt = position.shortAmount / 2;

If collateral_amount * collateral_price <= position_amount * mark_price * liquidation_ratio, the position is not healthy, and the liquidatable debt is computed.

The issue is the edge case ==: in such case, the position is considered not healthy.

The problem is that canLiquidate, for the same case, returns false:

File: src/ShortCollateral.sol
207: uint256 minCollateral = markPrice.mulDivUp(position.shortAmount, collateralPrice);
208:         minCollateral = minCollateral.mulWadDown(collateral.liqRatio);
209: 
210:         return position.collateralAmount < minCollateral

Impact

This mismatch means canLiquidate will return an incorrect value at the edge case position.collateral == minCollateralRequired. Any user/contract querying this getter before deciding whether to add collateral will expose themselves to liquidation. For instance:

if (!positionToken.canLiquidate(id)) return

Proof of Concept

Add this test to Exchange.Liquidation.t.sol, recreating the steps described above:

40: function testLiquidationLogicError() public {
41:         uint256 currentTime = block.timestamp;
42:         uint256 amount = 10e18;
43:         uint256 expectedPrice = initialPrice.mulDivDown(initialPrice, exchange.PRICING_CONSTANT());
44:         uint256 collateralAmount = expectedPrice * 125 / 100; // 151% Collateral ratio
45:         collateralAmount = collateralAmount.mulWadDown(amount);
46: 
47:          (uint256 positionId,) = openShort(amount, collateralAmount, user_1);
48: 
49:          bool isLiquidatable = shortCollateral.canLiquidate(positionId);
50:          //@audit position stated as healthy
51:          assertFalse(isLiquidatable);
52:          //@audit but maxDebt > 0 -> `liquidate` will go through
53:          uint256 maxDebt = shortCollateral.maxLiquidatableDebt(positionId);
54:          bool isDebtPositive = maxDebt > 0
55:          assertTrue(isDebtPositive);
56:          
57: 
58:         (uint256 markPrice,) = exchange.getMarkPrice();
59:         uint256 maxCost = maxDebt.mulWadDown(markPrice) * 105 / 100;
60:         openLong(maxDebt, maxCost, user_3);
61: 
62:         (markPrice,) = exchange.getMarkPrice();
63:         uint256 collateralLost = markPrice.mulWadDown(maxDebt);
64:         collateralLost = collateralLost * 105 / 100;
65: 
66:         uint256 preSusdBalance = susd.balanceOf(user_3);
67:         //@audit liquidation goes through
68:         vm.startPrank(user_3);
69:         exchange.liquidate(positionId, maxDebt);
70:         vm.stopPrank();
}

To simplify the values used and the test, make the following changes:

to TestSystem.sol

-56: uint256 public susdLiqRatio = 1.21e18;
+56: uint256 public susdLiqRatio = 1.25e18;

to ShortCollateral.sol

-54: require(susdRatio > susdLiqRatio);
+54: require(susdRatio >= susdLiqRatio);

This uses the same ratio for collateral and liquidation, so that the test can be simpler.

Tools Used

Manual Analysis, Foundry

Mitigation

Ensure canLiquidate matches the behavior of maxLiquidatableDebt, for instance:

File: src/ShortCollateral.sol
207: uint256 minCollateral = markPrice.mulDivUp(position.shortAmount, collateralPrice);
208:         minCollateral = minCollateral.mulWadDown(collateral.liqRatio);
209: 
-210:         return position.collateralAmount < minCollateral
+210:         return position.collateralAmount <= minCollateral

Report

Low Issues

L-1 Use require instead of assert

As per Solidity’s documentation: "The assert function creates an error of type Panic(uint256). The same error is created by the compiler in certain situations as listed below. Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix"

Instances (2):

File: src/LiquidityPool.sol

220:         assert(queuedDepositHead + count - 1 < nextQueuedDepositId);

285:         assert(queuedWithdrawalHead + count - 1 < nextQueuedWithdrawalId);

L-2 Setters should ensure contract address implement appropriate interfaces

Because the function can only be used once, ensure correctness of the vault_ by checking it implement a KangarooVault interface (which should be defined as it does not exist currently)

File: src/VaultToken.sol
35: function setVault(address _vault) external {
36:         if (vault != address(0x0)) {
37:             revert();
38:         }
39:         vault = _vault;
40:     }

L-3 Minor loss of precision when multiplying after dividing

Because Solidity truncates divisions, multiplication after division should be avoided

File: src/ShortCollateral.sol
169:        collateralAmt = markPrice.mulDivDown(shortAmount, collateralPrice);
170:        collateralAmt = collateralAmt.mulWadDown(coll.collateralRatio);

There can be some truncation in shortAmount * markPrice / collateralPrice, which is multiplied afterwards to coll.collateralRatio

L-4 Use _safeMint instead of _mint for ERC721 tokens

This will ensure that the recipient is either an EOA or implements IERC721Receiver

Instances (1):

File: src/ShortToken.sol

67:             _mint(trader, positionId);

L-5 Check sender in KangarooVault.receive()

KangarooVault currently transfers ETH received to feeReceipient. This means a user that mistakenly transfers ETH to it will lose that amount. On the other hand, there is a function saveToken for ERC20 tokens that would get stuck in the contract.

Instances (1):

File: src/KangarooVault.sol

455:         (bool success,) = feeReceipient.call{value: msg.value}("");

For consistency with the behavior of saveToken:

• refactor receive() so that it reverts
• create a function fundFeeRecipient to transfer ETH to feeReceipient

Non Critical Issues

NC-1 Remove unused variables

Instances (1):

File: src/Exchange.sol

50:     ERC20 public SUSD;

NC-2 Constants defined more than once

Some constants are defined twice or more in different contracts. Define them in only one contract, for instance using a constants library, so that values cannot become out of sync when only one location is updated

Instances (3):

SUSD, marketKey and systemManager

File: src/KangarooVault.sol

66:     ERC20 public immutable SUSD;

File: src/LiquidityPool.sol

65:     ERC20 public immutable SUSD;

File: src/PowerPerp.sol

9:     bytes32 public immutable marketKey;

10:     ISystemManager public immutable systemManager;

File: src/ShortCollateral.sol

46:     ISystemManager public immutable systemManager;

File: src/ShortToken.sol

9:     bytes32 public immutable marketKey;

11:     ISystemManager public immutable systemManager;

File: src/SystemManager.sol


24:     ERC20 public immutable SUSD;

File: src/utils/PauseModifier.sol

8:     ISystemManager public systemManager;

NC-3 Constants on the left are better

This is safer in case you forget to put a double equals sign, as the compiler wil throw an error instead of assigning the value to the variable.

Instances (42):

File: src/Exchange.sol

236:             require(shortPositions == 0, "Short position must be closed before opening");

248:             require(holdings == 0, "Long position must be closed before opening");

257:                 require(params.collateral == shortPosition.collateral);

291:             require(shortPositions == 0, "Shouldn't have short positions to close long postions");

302:             require(holdings == 0, "Shouldn't have long positions to close short postions");

308:             require(shortPosition.collateral == params.collateral);

File: src/KangarooVault.sol

188:         if (positionData.positionId == 0) {

219:         if (positionData.positionId == 0) {

249:             if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {

275:             if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minWithdrawDelay) {

281:             if (availableFunds == 0) {

342:         if (totalFunds == 0) {

347:         if (positionData.positionId == 0) {

557:         require(positionData.pendingLongPerp == 0 && positionData.pendingShortPerp == 0);

557:         require(positionData.pendingLongPerp == 0 && positionData.pendingShortPerp == 0);

560:         require(delayedOrder.sizeDelta == 0);

590:         if (positionData.positionId == 0) {

614:         require(positionData.pendingLongPerp > 0 && positionData.pendingShortPerp == 0);

617:         require(delayedOrder.sizeDelta == 0);

658:             if (positionData.shortAmount == 0) {

671:         require(positionData.positionId != 0 && positionData.pendingLongPerp == 0 && positionData.pendingShortPerp == 0);

671:         require(positionData.positionId != 0 && positionData.pendingLongPerp == 0 && positionData.pendingShortPerp == 0);

674:         require(delayedOrder.sizeDelta == 0);

731:         require(positionData.pendingLongPerp == 0 && positionData.pendingShortPerp > 0);

734:         require(delayedOrder.sizeDelta == 0);

774:             if (positionData.shortAmount == 0) {

File: src/LiquidityPool.sol

173:         require(msg.sender == address(exchange));

226:             if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {

291:             if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minWithdrawDelay) {

297:             if (availableFunds == 0) {

341:         if (totalFunds == 0) {

348:         if (skew == 0) {

695:         require(order.sizeDelta == 0);

File: src/LiquidityToken.sol

24:         require(msg.sender == liquidityPool);

File: src/PowerPerp.sol

28:         require(msg.sender == exchange);

File: src/ShortCollateral.sol

73:         require(msg.sender == address(exchange));

94:         if (userCollateral.collateral == address(0x0)) {

210:         return position.collateralAmount < minCollateral;

File: src/ShortToken.sol

39:         require(msg.sender == exchange);

55:         if (positionId == 0) {

69:             require(trader == ownerOf(positionId));

82:             if (position.shortAmount == 0) {

NC-4 Typos

File: src/KangarooVault.sol
182: /// @param amount Amount of sUSD being depositted//@audit-info deposited

NC-5 Order of functions not following Solidity standard practice

Follow Solidity's style guide for function ordering: constructor, receive/fallback, external, public, internal and private.

Instances (4):

File: src/Exchange.sol

206:     function orderFee(int256 sizeDelta) public view returns (uint256 fees) {

216:     function setSkewNormalizationFactor(uint256 _skewNormalizationFactor) external requiresAuth {

223:     function setMaxFundingRate(uint256 _maxFundingRate) external requiresAuth {

File: src/LiquidityPool.sol

153:     constructor(ERC20 _susd, bytes32 _baseAsset, bytes32 _trackingCode, ISystemManager _systemManager)

163:     function refresh() public { //@audit-info external functions defined after

184:     function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {

200:     function queueDeposit(uint256 amount, address user)

219:     function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {

247:     function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {

264:     function queueWithdraw(uint256 tokens, address user)

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

340:     function getTokenPrice() public view override returns (uint256) {

367:     function getSlippageFee(int256 sizeDelta) public view returns (uint256) {

379:     function orderFee(int256 sizeDelta) public view override returns (uint256) {

399:     function baseAssetPrice() public view override returns (uint256 spotPrice, bool isInvalid) {

404:     function getMarkPrice() public view override returns (uint256, bool) { //@audit-info external functions defined after

409:     function getSkew() external view override returns (int256) {

414:     function getDelta() external view override returns (uint256) {

419:     function getExposure() external view returns (int256) {

NC-6 Address receiving ETH should be payable

Low level calls can be performed on non-payable addresses, even when sending value. But to follow best practice (see OpenZeppelin's Address library) and add an extra layer of safety during compilation, consider casting these addresses as payable

Instances (2):

File: src/KangarooVault.sol

455:         (bool success,) = feeReceipient.call{value: msg.value}("");

File: src/LiquidityPool.sol

709:         (bool success,) = feeReceipient.call{value: msg.value}("");

NC-7 nonReentrant should be the first modifier

This ensures there is no reentrancy happening in other modifiers (even access control ones)

Instances (19):

File: src/KangarooVault.sol

376:     function openPosition(uint256 amt, uint256 minCost) external requiresAuth nonReentrant {

383:     function closePosition(uint256 amt, uint256 maxCost) external requiresAuth nonReentrant {

389:     function clearPendingOpenOrders(uint256 maxCost) external requiresAuth nonReentrant {

395:     function clearPendingCloseOrders(uint256 minCost) external requiresAuth nonReentrant {

401:     function transferPerpMargin(int256 marginDelta) external requiresAuth nonReentrant {

424:     function addCollateral(uint256 additionalCollateral) external requiresAuth nonReentrant {

436:     function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {

450:     function executePerpOrders(bytes[] calldata priceUpdateData) external payable requiresAuth nonReentrant {

File: src/LiquidityPool.sol

184:     function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {

219:     function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {

247:     function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {

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

557:     function liquidate(uint256 amount) external override onlyExchange nonReentrant {

568:     function hedgePositions() external override requiresAuth nonReentrant {

591:     function rebalanceMargin(int256 marginDelta) external requiresAuth nonReentrant {

613:     function increaseMargin(uint256 additionalMargin) external requiresAuth nonReentrant {

692:     function placeQueuedOrder() external requiresAuth nonReentrant {

704:     function executePerpOrders(bytes[] calldata priceUpdateData) external payable requiresAuth nonReentrant {

File: src/ShortCollateral.sol

106:     function sendCollateral(uint256 positionId, uint256 amount) external override onlyExchange nonReentrant {