Zaros Part 1

Zaros

DeFiFoundry

60,000 USDC

View results

Previous Next

Submission Details

Severity: high

Invalid

Updating market position decreases positions PnL

krisrenzo

Summary

Trading account PnL decreases when users update their positions.

Vulnerability Details

Zaros users can update their position by creating market orders for the order keeper to execute. Positions with greater than zero size delta are considered long and those lesser than zero, are short. To increase a position, traders just need to create another order with a size delta of the same (-ve / +ve) direction/sign as their position. When a user updates a position, their unrealized PnL is expected to be settled and reset back to zero (break-even). However, the protocol fails to implement this correctly as shown by the PoC below.

Proof of Code

The PoC below shows that an account uPnL decreases when they update their account even without a price change.

How to run the PoC

Create a new test file named BugPoC.t.sol in test/ folder of the contest repo
Copy the code below and paste it into the file.
Run the commands above the test functions in your CLI to run the test.

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.25;

// 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 {

///////////// 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 //////////////

////////// testPnLDecreaseAfterPriceUpdate ////////////

// forge test --match-test "testPnLDecreaseAfterPriceUpdate" -vvv

// broken invariant: performOrderExecution: account should not be in profit or loss when an order is created

function testPnLDecreaseAfterPriceUpdate() public {

vm.warp(1 hours);

vm.startPrank(BOB);

tradingAccountBranch_createTradingAccount(30_000e18, 2_000e18);

vm.stopPrank();

(, int256 btcPrice1,,,) = btcPriceFeed.latestRoundData();

console2.log("BTC price 1: ", uint256(btcPrice1));

vm.warp(1 hours);

uint256 activeMarketId_0 = perpsEngine.workaround_getActiveMarketsIdsLength(1);

console2.log("activeMarketIdLength 0: ", activeMarketId_0); // should be 0

vm.startPrank(BOB);

tradingAccountBranch_depositMargin(1, USDC, 10000e6, 100_000e18, 10_000e18); // deposit 10000e6 USDC

vm.stopPrank();

vm.warp(1 hours);

vm.startPrank(BOB);

orderBranch_createMarketOrder(1, true, 1e18, true, 100_000e18, 10_000e18); // create Market order of 1e18 BTC @ 100_000e18 USDC : 10x leverage

uint256 activeMarketId_1 = perpsEngine.workaround_getActiveMarketsIdsLength(1); // should be 1

console2.log("activeMarketIdLength 1: ", activeMarketId_1);

uint256 price = getPriceUint(btcPriceFeed);

Position.State memory before_positionState = perpsEngine.getPositionState(1, 1, price); // getPositionState(tradingAccountId, marketId, price)

console2.log("price 0: ", price);

console2.log("unrealizedPnlUsdX18 before: ", ((before_positionState.unrealizedPnlUsdX18).intoUD60x18()).intoUint256());

/// this assertion correctly passes as the position not expect to have any profit or loss immediately after its created

// assertTrue(before_positionState.unrealizedPnlUsdX18 == SD59x18_ZERO, "performOrderExecution: account should not be in profit or loss when an order is created");

vm.warp(1 hours);

vm.startPrank(BOB);

orderBranch_createMarketOrder(1, true, 1e18, true, 100_000e18, 10_000e18); // create Market order of 1e18 BTC @ 100_000e18 USDC : 10x leverage

vm.stopPrank();

uint256 activeMarketId_2 = perpsEngine.workaround_getActiveMarketsIdsLength(1);

console2.log("activeMarketIdLength 2: ", activeMarketId_2); // should be 0

uint256 price1 = getPriceUint(btcPriceFeed);

Position.State memory after_positionState = perpsEngine.getPositionState(1, 1, price1); // getPositionState(tradingAccountId, marketId, price)

console2.log("price 1: ", price1);

// without using the absolute value of unrealizedPnlUsdX18, the convertion from SD59x18 to uint256 will revert with underflow

console2.log("unrealizedPnlUsdX18 after: ", ((after_positionState.unrealizedPnlUsdX18.abs()).intoUD60x18()).intoUint256());

/// this assertion incorrectly fails as the position uPnL unexpectedly decreases.

// Even though the PnL should have been settled and reset after the second order was filled

// Also note that the price didn't change, yet somehow the position uPnL decreased

assertTrue(after_positionState.unrealizedPnlUsdX18 == SD59x18_ZERO, "performOrderExecution: account should not be in profit or loss when an order is created");

// this assertion incorrectly passes as the position uPnL unexpectedly decrease.

// assertTrue(after_positionState.unrealizedPnlUsdX18 < SD59x18_ZERO, "performOrderExecution: account should not be in profit or loss when an order is created");

}

</details>

Impact

This can lead to false liquidation of trading accounts

Tools Used

Foundry

Recommendations

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago

Submission Judgement Published

Invalidated

Reason: Too generic

Appeal created

krisrenzo Submitter

10 months ago

inallhonesty Lead Judge

10 months ago

inallhonesty Lead Judge 10 months ago

Submission Judgement Published

Invalidated

Reason: Too generic

Prize pool breakdown

Total prize

60,000 USDC

nSLOC

2,878

21 USDC / LOC

High Medium

50,000 USDC

Low

5,500 USDC

Judges

4,500 USDC

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.