Polynomial Protocol contest - RaymondFam's results

Lines of code

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

Vulnerability details

Impact

In ShortCollateral.sol, the slash logic of maxLiquidatableDebt() is specifically too harsh to the barely unhealthy positions because maxDebt will be half of the position to be liquidated if 0.95e18 <= safetyRatio <= 1e18.

Additionally, once a position turns liquidatable, the position is deemed fully liquidatable atomically in two repeated transactions.

Proof of Concept

Supposing we resort to the following setup as denoted in ShortCollateral.t.sol:

collRatio = 1.5e18 liqRatio = 1.3e18 liqBonus = 1e17

Collateral ratio of a position, x = (position.collateralAmount * collateralPrice) / (position.shortAmount * markPrice)

File: ShortCollateral.sol#L230-L239

        uint256 safetyRatioNumerator = position.collateralAmount.mulWadDown(collateralPrice);
        uint256 safetyRatioDenominator = position.shortAmount.mulWadDown(markPrice);
        safetyRatioDenominator = safetyRatioDenominator.mulWadDown(collateral.liqRatio);
        uint256 safetyRatio = safetyRatioNumerator.divWadDown(safetyRatioDenominator);

        if (safetyRatio > 1e18) return maxDebt;

        maxDebt = position.shortAmount / 2;

        if (safetyRatio < WIPEOUT_CUTOFF) maxDebt = position.shortAmount;

According to the code block above with liqRatio factored in:

In order to avoid being liquidated, a position will need to have a collateral ratio, x > 1.3e18 so that safetyRatio > (1.3 / 1.3)e18 which is safetyRatio > 1e18.

The position will be half liquidated if its associated collateral ratio falls in the range of 1.235e18 <= x <= 1.3e18. To avoid full liquidation, the condition at the lower end will need to be safetyRatio >= (1.235 / 1.3)e18 which is safetyRatio >= 0.95e18.

The position will be fully liquidated if x < 1.235e18.

Here is the unforgiving scenario:

1. Bob has a short position whose collateral ratio happens to be 1.3e18.

2. Bob's position gets half liquidated the first round ending up with a collateral ratio, x (Note: The numerator is multiplied by 0.45 because of the additional 10% liqBonus added to the one half collateral slashed:

(1.3 * 0.45 / 0.5)e18 = 1.17e18

3. Immediately, Bob's position becomes fully liquidatable because x < 1.235e18.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider restructuring the slashing logic such that the position turns healthy after being partially liquidated, instead of making it spiral down to the drain.

    (uint256 markPrice,) = exchange.getMarkPrice();
    (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);
    uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice);
    uint256 liqBonus = collateralClaim.mulWadDown(coll.liqBonus);
    totalCollateralReturned = liqBonus + collateralClaim;

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L241 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L295 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L350 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/PowerPerp.sol#L32-L38

Vulnerability details

Impact

contract PowerPerp that has Solmate's ERC20.sol inherited has zero totalSupply initiated by default. And, as denoted by PowerPerp.sol, onlyExchange can mint or burn Power (Square) Perp ERC-20 tokens:

File: PowerPerp.sol#L32-L38

    function mint(address _user, uint256 _amt) public onlyExchange {
        _mint(_user, _amt);
    }

    function burn(address _user, uint256 _amt) public onlyExchange {
        _burn(_user, _amt);
    }

This could run into supply issue if the source and drain have not been routed through the right channels.

Proof of Concept

There is only one source of minting when users open long trades.

However, there are two channels users could burn the tokens, i.e. closing long trades, and liquidating positions.

It is apparent that the long traders hold the majority of the tokens although long term the tokens can be swapped for SUSD on the swap exchange, believing that is how MarkPrice derives from. Additionally, this should be where liquidators who are neither long nor short traders buy their Power Perp tokens from prior to profiting on the liquidable positions.

If liquidations get more frequent than expected due to collateral token dropping in price, powerPerp.burn() is going to both strain the Power Perp token supply and drive up the price. As a result, the amount of positions going underwater is going to increase too.

The chain effect continues since long traders will have difficulty closing their positions if their minted Power Perp tokens have earlier been swapped for SUSD at lower prices than now.

The situation could be worse if the scenario described above were to happen in the early phase of system launch.

Tools Used

Manual inspection

Recommended Mitigation Steps

Since short traders are sent the cost deducted SUSD when opening their positions, consider having liquidators sending in the equivalent amount of cost added SUSD to the liquidity pool (just like when short traders are closing their positions) instead of having Power Perp tokens burned in _liquidate(). This will also have a good side effect of enhancing the hedging capability of the liquidity pool.

Lines of code

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

Vulnerability details

Impact

When completely closing a short trade, a user is supposed to input TradeParams such that:

shortPosition.shortAmount == params.shortAmount shortPosition.collateralAmount == params.collateralAmount

However, if cares have not been given particularly when inputting params.collateralAmount via a non-frontend method such as https://optimistic.etherscan.io/, a zero or a value smaller than shortPosition.collateralAmount could be accidentally entered. After the transaction has succeeded, the user's collateral would be permanently locked in contract ShortCollateral.

Proof of Concept

As can be seen from the code block pertaining to _closeTrade() below, totalShortAmount == 0 will make the require statement pass easily because minCollateral == 0.

File: Exchange.sol#L310-L319

            uint256 totalShortAmount = shortPosition.shortAmount - params.amount;
            uint256 totalCollateralAmount = shortPosition.collateralAmount - params.collateralAmount;

            uint256 minCollateral = shortCollateral.getMinCollateral(totalShortAmount, params.collateral);
            require(totalCollateralAmount >= minCollateral, "Not enough collateral");

            shortCollateral.sendCollateral(params.positionId, params.collateralAmount);
            shortToken.adjustPosition(
                params.positionId, msg.sender, params.collateral, totalShortAmount, totalCollateralAmount
            );

The inadequate params.collateralAmount accidentally inputted is then sent to the user:

File: ShortCollateral.sol#L106-L116

    function sendCollateral(uint256 positionId, uint256 amount) external override onlyExchange nonReentrant {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        userCollateral.amount -= amount;

        address user = shortToken.ownerOf(positionId);

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

        emit SendCollateral(positionId, userCollateral.collateral, amount);
    }

Next, the user's position is adjusted such that its position is burned because of a complete position close. Note that position.shortAmount is assigned 0 whereas position.collateralAmount is assigned a non-zero value.

File: ShortToken.sol#L79-L84

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

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

Because the user's ERC-721 short token is now burned, removing the forgotten/remaining collateral from the short position is going to revert on the ownership check:

File: Exchange.sol#L388

        require(shortToken.ownerOf(positionId) == msg.sender);

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider adding a check in _closeTrade() that will fully send the collateral to the user when the position is intended to be fully closed as follows:

File: Exchange.sol#L310-L316

            uint256 totalShortAmount = shortPosition.shortAmount - params.amount;
            uint256 totalCollateralAmount = shortPosition.collateralAmount - params.collateralAmount;

            uint256 minCollateral = shortCollateral.getMinCollateral(totalShortAmount, params.collateral);
            require(totalCollateralAmount >= minCollateral, "Not enough collateral");

+            if (totalShortAmount == 0) {
+                params.collateralAmount = shortPosition.collateralAmount;
+            }
            shortCollateral.sendCollateral(params.positionId, params.collateralAmount);

Use practical code execution instead of assert()

In LiquidityPool.sol, the assert statement of processDeposits() and processWithdraws() may be replaced with a simple code logic that calculates count that will also help circumvent the overvalued input of count that reverts.

For instance, the following function may be refactored as follows:

File: LiquidityPool.sol#L219-L242

-    function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {
+    function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {
-        assert(queuedDepositHead + count - 1 < nextQueuedDepositId);
+        uint256 count = nextQueuedDepositId - queuedDepositHead;
        uint256 tokenPrice = getTokenPrice();

        for (uint256 i = 0; i < count; i++) {
            QueuedDeposit storage current = depositQueue[queuedDepositHead];

            if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {
                return;
            }

            uint256 tokensToMint = current.depositedAmount.divWadDown(tokenPrice);

            current.mintedTokens = tokensToMint;
            totalQueuedDeposits -= current.depositedAmount;
            totalFunds += current.depositedAmount;
            liquidityToken.mint(current.user, tokensToMint);

            emit ProcessDeposit(current.id, current.user, current.depositedAmount, tokensToMint, current.requestedTime);

            current.depositedAmount = 0;
            queuedDepositHead++;
        }
    }

Uninitialized variables

In LiquidityPool.sol, Exchange.sol, and KangarooVault.sol, consider initializing the settable variables in the constructor just to make sure function calls dependent on them are not going to break or malfunction due to calls being made before these variables manage to be set.

For instance, for failing failing to set baseTradingFee in time, it is going to directly affect getSlippageFee(), which in turns affects orderFee(), openLong(), closeLong(), openShort(), and closeShort() respectively.

Although this can also be circumvented by PauseModifier upon contract deployment, it will require added work to maneuver since systemManager.isPaused(key) == false by default.

Missing availableFunds check

In LiquidityPool.sol, consider adding the availableFunds check in withdraw() just as it has been done so in processWithdraws(). Otherwise, users could dodge it to drain the contract SUSD balance when it should not be, i.e. totalFunds < uint256(usedFunds).

Note: There has been a logic flaw in the check of which I have separately submitted the vulnerability in a different report.

Missing fee provision for depositors in KangarooVault.sol

Consider implementing devFee on withdrawalFee like it has been done so in LiquidityPool.sol so that liquidity providers, the core counterparts to the trade market, will be more incentivized providing liquidity to the vault.

Missing upper bound check on deposit amount

In KangarooVault.sol, maxDepositAmount has been set but never implemented. Consider including it in initiateDeposit():

File: KangarooVault.sol#L185

        require(amount >= minDepositAmount);
+        require(amount <= maxDepositAmount);

Underflow check

Consider adding an underflow check for availableFunds in processWithdrawalQueue() of KangarooVault.sol.

File: KangarooVault.sol#L279

+            require (totalFunds >= usedFunds);
            uint256 availableFunds = totalFunds - usedFunds;

Zero value check at the constructor

Zero value checks should be implemented at the constructor to avoid accidental error(s) that could result in non-functional calls associated with it particularly when assigning immutable variables.

For instance, the constructor below may be refactored as follows:

File: Exchange.sol#L67-L72

    constructor(uint256 _priceNormalizationFactor, ISystemManager _systemManager)
        Auth(msg.sender, Authority(address(0x0)))
    {
+        require(_priceNormalizationFactor != 0);
        PRICING_CONSTANT = _priceNormalizationFactor;
        systemManager = _systemManager;
    }

Inheritance from interface

contract ShortToken should inherit from interface IShortToken just as it has been likewise done so in ShortCollateral.sol. This will spare the contract from needing to include the struct ShortPosition.

File: ShortToken.sol#L4-L22

import {ERC721} from "solmate/tokens/ERC721.sol";
import {ISystemManager} from "./interfaces/ISystemManager.sol";
import {IPowerPerp} from "./interfaces/IPowerPerp.sol";
+import {IShortToken} from "./interfaces/IShortToken.sol";

- contract ShortToken is ERC721 {
+ contract ShortToken is ERC721, IShortToken {
    bytes32 public immutable marketKey;
    address public exchange;
    ISystemManager public immutable systemManager;
    IPowerPerp public powerPerp;

    uint256 public nextId = 1;
    uint256 public totalShorts;

-    struct ShortPosition {
-        uint256 positionId;
-        uint256 shortAmount;
-        uint256 collateralAmount;
-        address collateral;
-    }

Devoid of system documentation and complete technical specification

A system’s design specification and supporting documentation should be almost as important as the system’s implementation itself. Users rely on high-level documentation to understand the big picture of how a system works. Without spending time and effort to create palatable documentation, a user’s only resource is the code itself, something the vast majority of users cannot understand. Security assessments depend on a complete technical specification to understand the specifics of how a system works. When a behavior is not specified (or is specified incorrectly), security assessments must base their knowledge in assumptions, leading to less effective review. Maintaining and updating code relies on supporting documentation to know why the system is implemented in a specific way. If code maintainers cannot reference documentation, they must rely on memory or assistance to make high-quality changes. Currently, the only documentation available is a single README file, as well as code comments.

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Consider fully equipping all contracts with complete set of NatSpec to better facilitate users/developers interacting with the protocol's smart contracts.

For example, the following function instance could be better equipped with its missing @param referralCode and @return totalCost:

File: LiquidityPool.sol#L427-L436

    /// @notice Called by exchange when a new long position is created
    /// @param amount Amount of square perp being longed
    /// @param user Address of the user
    function openLong(uint256 amount, address user, bytes32 referralCode)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCost)
    {

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

On chain over off chain implementation of referralCode

referralCode currently works as a function parameter serving only to be emitted in the trading methods. Consider adopting a brief but comprehensive set of on chain logic that will make it permissionless instead of handling the affiliate programs off chain via backend API logging.

Unspecific compiler version pragma

For some source-units the compiler version pragma is very unspecific, i.e. ^0.8.9. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Use a more recent version of solidity

The protocol adopts version 0.8.9 on writing contracts. For better security, it is best practice to use the latest Solidity version, 0.8.17.

Security fix list in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Solidity Compiler optimization could be problematic

hardhat.config.js:
  29  module.exports = {
  30:   solidity: {
  31:     compilers: [
  32:       {
  33:         version: "0.8.9",
  34:         settings: {
  35:           optimizer: {
  36:               enabled: true,
  37:               runs: 1000000
  38
            }

Description: The protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.

Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.

Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.

Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a[x] = false instance below may be refactored as follows:

File: KangarooVault.sol#L719

-        lastTradeData.isOpen = false;
+        delete lastTradeData.isOpen;

Tokens accidentally sent to the contract cannot be recovered

It is deemed unrecoverable if the tokens accidentally arrive at the contract addresses, which has happened to many popular projects. Consider adding a recovery code to your critical contracts.

Consider adding saveToken() in LiquidityPool.sol and/or Exchange.sol where possible.

Gas griefing/theft is possible on unsafe external call

return data (bool success,) has to be stored due to EVM architecture, if in a usage like below, ‘out’ and ‘outsize’ values are given (0,0). Thus, this storage disappears and may come from external contracts a possible gas grieving/theft problem is avoided as denoted in the link below:

https://twitter.com/pashovkrum/status/1607024043718316032?t=xs30iD6ORWtE2bTTYsCFIQ&s=19

Here are the two instances entailed:

File: KangarooVault.sol#L454-L457 File: LiquidityPool.sol#L708-L713

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

More in-depth tests needed

Consider extending the test suites to include edge cases, and where possible implement fuzzing that will often unfold many unexpected incidents that could break. This will greatly make the code safer and more robust to vulnerability attacks.

Comment and code mismatch

File: LiquidityPool.sol#L86-L87

-    /// @notice Minimum deposit delay
+    /// @notice Minimum withdrawal delay
    uint256 public minWithdrawDelay;

Efficient code usage

In Exchange.sol, getMarkPrice() and _updateFundingRate() share a big chunk of identical code. Consider making a minor restructure to have the repeated code internally invoked that will have a good side effect of making fundingLastUpdated more recently updated.

-    function _updateFundingRate() internal {
+    function _updateFundingRate() internal returns (uint256 normalizationFactor) {
        (int256 fundingRate,) = getFundingRate();
        fundingRate = fundingRate / 1 days;

        int256 currentTimeStamp = int256(block.timestamp);
        int256 fundingLastUpdatedTimestamp = int256(fundingLastUpdated);

        int256 totalFunding = wadMul(fundingRate, (currentTimeStamp - fundingLastUpdatedTimestamp));
        int256 normalizationUpdate = 1e18 - totalFunding;

        normalizationFactor = normalizationFactor.mulWadDown(uint256(normalizationUpdate));

        emit UpdateFundingRate(fundingLastUpdated, normalizationFactor);

        fundingLastUpdated = block.timestamp;
    }

    function getMarkPrice() public view override returns (uint256 markPrice, bool isInvalid) {
        (uint256 baseAssetPrice, bool invalid) = pool.baseAssetPrice();
        isInvalid = invalid;

-        (int256 fundingRate,) = getFundingRate();
-        fundingRate = fundingRate / 1 days;

-        int256 currentTimeStamp = int256(block.timestamp);
-        int256 fundingLastUpdatedTimestamp = int256(fundingLastUpdated);

-        int256 totalFunding = wadMul(fundingRate, (currentTimeStamp - fundingLastUpdatedTimestamp));
-        int256 normalizationUpdate = 1e18 - totalFunding;
-        uint256 newNormalizationFactor = normalizationFactor.mulWadDown(uint256(normalizationUpdate));

+        uint256 newNormalizationFactor = _updateFundingRate();

        uint256 squarePrice = baseAssetPrice.mulDivDown(baseAssetPrice, PRICING_CONSTANT);
        markPrice = squarePrice.mulWadDown(newNormalizationFactor);
    }