Submission Details
Severity:
Incorrect implementation of PerpMarket::getMarkPrice can lead incorrect mark prices
Summary
The logic in PerpMarket::getMarkPrice is faulty as it can result to an correct markPrice
Vulnerability Details
Zaros correctly utilizes mark price over index price for quoting asset prices, as mark price factors in the skew of the market. However, the function responsible for calculating this mark price is incorrect as it breaks certain invariants necessary for correct operation.
Key definitions
skew: a measure of how far or close the price of a derivative is to the price of the underlying asset. The closer to zero the skew is, the closer to the index price the derivative price is.
index price: the real price of the underlying asset. Zaros source this from chainlink and other offchain oracles.
mark price: the price of the direvative in zaros after the skew has been factored in.
Key invariants related to this submission
When skew is less than zero (short bias), mark price should be less than index price to encourage buying activities to push it back towards the index price
When skew is greater than zero (long bias), mark price should be greater than index price to encourage selling activities to push it back towards the index price
Proof of Code
The PoC below shows that the invariants mentioned above are easily broken. i.e. mark price below lower or higher than index price when it should have been the other way around.
How to run the PoC
Create a new test file named
SubmissionPoC.t.solintest/folder of the contest repoCopy the code below and paste in the file.
Run the commands above the test functions in your CLI to run the test.
</details>
// SPDX-License-Identifier: UNLICENSED
// Zaros dependencies
import { Base\_Test } from "test/Base.t.sol";
import { Errors } from "@zaros/utils/Errors.sol";
import { ERC20, IERC20 } from "@openzeppelin/token/ERC20/ERC20.sol";
import {console2} from "forge-std/console2.sol";
import { MockERC20 } from "test/mocks/MockERC20.sol";
import { MockPriceFeed } from "test/mocks/MockPriceFeed.sol";
import { Users, User, MockPriceAdapters } from "test/utils/Types.sol";
import { UpgradeBranch } from "@zaros/tree-proxy/branches/UpgradeBranch.sol";
import { LookupBranch } from "@zaros/tree-proxy/branches/LookupBranch.sol";
import { GlobalConfigurationBranch } from "@zaros/perpetuals/branches/GlobalConfigurationBranch.sol";
import { LiquidationBranch } from "@zaros/perpetuals/branches/LiquidationBranch.sol";
import { OrderBranch } from "@zaros/perpetuals/branches/OrderBranch.sol";
import { PerpMarketBranch } from "@zaros/perpetuals/branches/PerpMarketBranch.sol";
import { TradingAccountBranch } from "@zaros/perpetuals/branches/TradingAccountBranch.sol";
import { SettlementBranch } from "@zaros/perpetuals/branches/SettlementBranch.sol";
import { MarginCollateralConfiguration } from "@zaros/perpetuals/leaves/MarginCollateralConfiguration.sol";
import { Position } from "@zaros/perpetuals/leaves/Position.sol";
import { MarketOrder } from "@zaros/perpetuals/leaves/MarketOrder.sol";
import { SettlementConfiguration } from "@zaros/perpetuals/leaves/SettlementConfiguration.sol";
import { TradingAccountBranchTestnet } from "testnet/branches/TradingAccountBranchTestnet.sol";
import { GlobalConfigurationHarness } from "test/harnesses/perpetuals/leaves/GlobalConfigurationHarness.sol";
import { MarginCollateralConfigurationHarness } from "test/harnesses/perpetuals/leaves/MarginCollateralConfigurationHarness.sol";
import { MarketConfigurationHarness } from "test/harnesses/perpetuals/leaves/MarketConfigurationHarness.sol";
import { MarketOrderHarness } from "test/harnesses/perpetuals/leaves/MarketOrderHarness.sol";
import { PerpMarketHarness } from "test/harnesses/perpetuals/leaves/PerpMarketHarness.sol";
import { PositionHarness } from "test/harnesses/perpetuals/leaves/PositionHarness.sol";
import { SettlementConfigurationHarness } from "test/harnesses/perpetuals/leaves/SettlementConfigurationHarness.sol";
import { TradingAccountHarness } from "test/harnesses/perpetuals/leaves/TradingAccountHarness.sol";
import {LiquidationKeeper} from "@zaros/external/chainlink/keepers/liquidation/LiquidationKeeper.sol";
import {MarketOrderKeeper} from "@zaros/external/chainlink/keepers/market-order/MarketOrderKeeper.sol";
// PRB Math dependencies
import { UD60x18, ud60x18, ZERO as UD60x18\_ZERO, convert as ud60x18Convert } from "@prb-math/UD60x18.sol";
import {SD59x18,sd59x18, unary, UNIT as SD\_UNIT, ZERO as SD59x18\_ZERO, convert as sd59x18Convert} from "@prb-math/SD59x18.sol";
contract BugPoC is Base\_Test {
```Solidity
///////////// SETUP //////////////
address[] internal USERS;
MockERC20[] internal EXTERNAL_TOKENS;
address internal BOB;
address internal ALICE;
address internal defaultUser; // defaultUser
address USDC;
address wBTC;
address weETH;
uint256 internal constant INITIAL_USDC_BALANCE = 1_000_000;
uint256 internal constant INITIAL_WEETH_BALANCE = 100_000;
uint256 internal constant INITIAL_WBTC_BALANCE = 10_000;
MockPriceFeed btcPriceFeed;
MockPriceFeed ethPriceFeed;
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarkets();
EXTERNAL_TOKENS = new MockERC20[](3);
EXTERNAL_TOKENS[0] = usdc;
EXTERNAL_TOKENS[1] = weEth;
EXTERNAL_TOKENS[2] = wBtc;
USDC = address(usdc);
wBTC = address(wBtc);
weETH = address(weEth);
btcPriceFeed = MockPriceFeed(marketsConfig[1].priceAdapter);
ethPriceFeed = MockPriceFeed(marketsConfig[2].priceAdapter);
BOB = users.naruto.account;
ALICE = users.sasuke.account;
USERS = new address[](2);
USERS[0] = BOB;
USERS[1] = ALICE;
_topUpUsers();
vm.stopPrank();
}
function _topUpUsers() internal {
address user;
for (uint8 i = 0; i < USERS.length; i++) {
user = USERS[i];
EXTERNAL_TOKENS[0].mint(user, INITIAL_USDC_BALANCE * (10 ** EXTERNAL_TOKENS[0].decimals()));
EXTERNAL_TOKENS[1].mint(user, INITIAL_WEETH_BALANCE * (10 ** EXTERNAL_TOKENS[0].decimals()));
EXTERNAL_TOKENS[2].mint(user, INITIAL_WBTC_BALANCE * (10 ** EXTERNAL_TOKENS[0].decimals()));
}
}
modifier updateMockPriceFeedBtcEth(uint256 _btcNewPrice, uint256 _ethNewPrice) {
updateMockPriceFeed(1, _btcNewPrice);
updateMockPriceFeed(2, _ethNewPrice);
_;
}
function getPriceUint(MockPriceFeed priceFeed) internal view returns (uint256) {
(, int256 answer,,,) = priceFeed.latestRoundData();
return uint256(answer);
}
function performLiquidation(uint128 nextAccountId) internal {
LiquidationKeeper liquidationKeeper_ = LiquidationKeeper(liquidationKeeper);
uint256 checkLowerBound = 0;
uint256 checkUpperBound = uint256(nextAccountId);
uint256 performLowerBound = 0;
uint256 performUpperBound = checkUpperBound + 1;
if (checkUpperBound < 1) return;
bytes memory data = abi.encode(checkLowerBound, checkUpperBound, performLowerBound, performUpperBound);
(bool upkeepNeeded, bytes memory performData) = liquidationKeeper_.checkUpkeep(data);
if (upkeepNeeded) {
vm.startPrank(users.keepersForwarder.account);
liquidationKeeper_.performUpkeep(performData);
vm.stopPrank();
}
}
struct TestHelper {
uint256 oldBtcPrice;
uint256 oldEthPrice;
uint256 btcNewPrice;
uint256 ethNewPrice;
uint256 price;
bytes32 streamId;
uint256 priceBtcChange;
uint256 priceEthChange;
OrderBranch.CreateMarketOrderParams params;
}
function _performOrderExecution(uint256 marketId, uint128 tradingAccountId, bool isIncrease) internal {
TestHelper memory helper;
helper.oldBtcPrice = getPriceUint(btcPriceFeed);
helper.oldEthPrice = getPriceUint(ethPriceFeed);
helper.btcNewPrice;
helper.ethNewPrice;
helper.price;
helper.streamId = marketsConfig[marketId].streamId;
helper.priceBtcChange = helper.oldBtcPrice % 100e18;
helper.priceEthChange = helper.oldEthPrice % 10e18;
if(isIncrease) {
helper.btcNewPrice = helper.oldBtcPrice + helper.priceBtcChange;
helper.ethNewPrice = helper.oldEthPrice + helper.priceEthChange;
} else {
helper.btcNewPrice = helper.oldBtcPrice - helper.priceBtcChange;
helper.ethNewPrice = helper.oldEthPrice - helper.priceEthChange;
}
btcPriceFeed.updateMockPrice(helper.btcNewPrice);
ethPriceFeed.updateMockPrice(helper.ethNewPrice);
address marketOrderKeeper_ = marketOrderKeepers[marketId];
MarketOrderKeeper marketOrderKeeper = MarketOrderKeeper(marketOrderKeeper_);
if (marketId == 1) {
helper.price = getPriceUint(btcPriceFeed);
} else {
helper.price = getPriceUint(ethPriceFeed);
}
bytes memory mockSignedReport = getMockedSignedReport(helper.streamId, helper.price);
vm.startPrank(users.owner.account);
marketOrderKeeper.setForwarder(users.keepersForwarder.account);
vm.stopPrank();
bytes memory performData = abi.encode(mockSignedReport, abi.encode(tradingAccountId));
vm.startPrank(users.keepersForwarder.account);
marketOrderKeeper.performUpkeep(performData);
vm.stopPrank();
}
function tradingAccountBranch_createTradingAccount(
uint256 _btcNewPrice,
uint256 _ethNewPrice
) public updateMockPriceFeedBtcEth(_btcNewPrice, _ethNewPrice) returns(uint128 _tradingAccountId){
return perpsEngine.createTradingAccount(bytes(""), false) ;
}
/// depositMargin
function tradingAccountBranch_depositMargin(
uint128 _tradingAccountId,
address collateralType,
uint256 _amount,
uint256 _btcNewPrice,
uint256 _ethNewPrice
) public updateMockPriceFeedBtcEth(_btcNewPrice, _ethNewPrice) {
perpsEngine.depositMargin(_tradingAccountId, collateralType, _amount);
}
/// withdrawMargin
function tradingAccountBranch_withdrawMargin(
uint128 _tradingAccountId,
address collateralType,
uint256 _amount,
uint256 _btcNewPrice,
uint256 _ethNewPrice
) public updateMockPriceFeedBtcEth(_btcNewPrice, _ethNewPrice) {
perpsEngine.withdrawMargin(_tradingAccountId, collateralType, _amount);
}
/// createMarketOrder
function orderBranch_createMarketOrder(
uint128 _tradingAccountId,
bool _buyBtc,
int128 sizeDelta,
bool isIncrease,
uint256 _btcNewPrice,
uint256 _ethNewPrice
) public updateMockPriceFeedBtcEth(_btcNewPrice, _ethNewPrice) {
TestHelper memory helper;
helper.params.tradingAccountId = _tradingAccountId;
helper.params.marketId = _buyBtc ? 1 : 2;
helper.params.sizeDelta = sizeDelta;
perpsEngine.createMarketOrder(helper.params);
_performOrderExecution(helper.params.marketId, helper.params.tradingAccountId, isIncrease);
}
///////////// TEST //////////////
struct TestGetMarkPrice {
uint256 skewScaleU256;
int128 skew0;
UD60x18 indexPriceX18;
SD59x18 skewDelta;
}
// faulty implementation of getMarkPrice causes it to return incorrect markPrices
// forge test --match-test "testGetMarkPrice" -vvv
function testGetMarkPrice()public{
////// Test data setup starts here /////////
TestGetMarkPrice memory helper;
helper.skewScaleU256 = 10_000_000e18; // same as the skewScale constant in the script
// indexPriceU256 = bound(indexPriceU256, 10_000e18, 1_000_000e18);
helper.indexPriceX18 = ud60x18(990000000000000000012766); // 990_000e18 -- 990_000 BTC/USD
// Note that the above price is not mandatory for the tests to work.
// It's just the one I picked from the fuzzer. The vuln appears over a wide price range
// currently a bullish market
helper.skew0 = int128(7802);
// new short trade that turns the market bearish
helper.skewDelta = sd59x18(-(int128(8604)));
//////// Contract's logic starts here /////////
SD59x18 skewScale = sd59x18(int256(helper.skewScaleU256));
SD59x18 skew = sd59x18(helper.skew0);
// priceImpactBeforeDelta and priceImpactAfterDelta are zero due to solidity's rounding
// Part A: division before multiplication
SD59x18 priceImpactBeforeDelta = skew.div(skewScale);
SD59x18 newSkew = skew.add(helper.skewDelta); // newSkew = 7802 + (-8604) = -802
SD59x18 priceImpactAfterDelta = newSkew.div(skewScale);
SD59x18 cachedIndexPriceX18 = helper.indexPriceX18.intoSD59x18();
// priceBeforeDelta and priceAfterDelta will be equal cachedIndexPriceX18 because
// priceImpactBeforeDelta and priceImpactAfterDelta are equal to zero due to their
// multiplication by priceImpactBeforeDelta, which is zero
// This means the protocol is report that the trade had no price impact, which is obviously incorrect
// Part B: division before multiplication
UD60x18 priceBeforeDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18();
UD60x18 priceAfterDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18();
// markPrice is incorrectly equal to indexPriceX18 as a result
// The only time maxPrice should be equal to indexPriceX18 is when the skew is zero
// And the only way the skew would be zero in this scenario is if the net sum of the sizeDelta and existing skew is zero
UD60x18 markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
// Contract's logic ends here
//////// Console log output and Assertions /////////
console2.log("index Price: ", helper.indexPriceX18.intoUint256());
console2.log("markPrice: ", markPrice.intoUint256());
assertTrue(markPrice == helper.indexPriceX18, "markPrice == indexPriceX18");
}
}
</details>
In addition, there is a division before multiplication in the PerpMarket::getMarkPrice which can further exercebate this issue. This is worth looking into on its own.
Point of division is here
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
view
returns (UD60x18 markPrice)
{
SD59x18 skewScale = sd59x18(self.configuration.skewScale.toInt256());
SD59x18 skew = sd59x18(self.skew);
@> SD59x18 priceImpactBeforeDelta = skew.div(skewScale); // division
SD59x18 newSkew = skew.add(skewDelta);
@> SD59x18 priceImpactAfterDelta = newSkew.div(skewScale); // division
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
@> UD60x18 priceBeforeDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18(); // multiplication
UD60x18 priceAfterDelta =
@> cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18(); // multiplication
markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
}
The skew is divided by skewScale before multiplying by cachedIndexPriceX18. The risk is that dividing before multiplying can result in a loss of precision in the final markPrice calculation. This is because the division operation truncates any decimal places.
Impact
Incorrect skew calculation affects many critical parts of the protocol including, price calculations, fee charges, and the general sustainance of the market and protocol.
Tools Used
Foundry
Recommendations
Review the division before multiplication error in the function and stress test the chosen constraints for the selected markets.
Updates
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.