Summary
When traders create orders, the actual fill price will be influenced by the current skew. Orders processed before the trader's order may influence the skew enough to make the intended skew and taker/maker fee change.
Vulnerability Details
Due to missign slippage checks on the fill price, users will be filling orders with unpleasant unexpected prices.
Let's say that Bob sees a favorable skew for long orders, Bob places a long order expecting a favorable skew and taker/maker fee. However Eve's long order got processed before Bob's and Bob ends up filling an order with an unfovarble skew instead.
Consider the contract below mimicking the markPrice logic:
pragma solidity >=0.8.19;
import { UD60x18, ud60x18, 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 MadeContract {
function getMarkPrice(
int256 skewDelta,
uint256 indexPriceX18,
int256 initialSkew
)
public
view
returns (UD60x18 markPrice)
{
SD59x18 skewDelta = sd59x18(skewDelta);
UD60x18 indexPriceX18 = ud60x18(indexPriceX18);
SD59x18 skewScale = sd59x18(100_000e18);
SD59x18 skew = sd59x18(initialSkew);
SD59x18 priceImpactBeforeDelta = skew.div(skewScale);
SD59x18 newSkew = skew.add(skewDelta);
SD59x18 priceImpactAfterDelta = newSkew.div(skewScale);
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
UD60x18 priceBeforeDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18();
UD60x18 priceAfterDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18();
markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
}
}
Now the POC below portrays a scenario where a trader A sees a *-100'000 * skew, therefore he puts a long order taking advantage of the current skew. However since great mind think alike, a fellow trader had the same idea, and was lucky enough to have his transaction processed before trader A's.
pragma solidity >=0.8.19;
import "forge-std/Test.sol";
import {MadeContract} from "../src/MadeContract.sol";
import { UD60x18, ud60x18, convert as ud60x18Convert, intoUint256} from "@prb-math/UD60x18.sol";
contract POCTest is Test {
MadeContract public target;
function setUp() public {
target = new MadeContract();
}
function testGetPrice() public {
UD60x18 intendedPrice = target.getMarkPrice(50_000e18, 60_000e18, -100_000e18);
uint256 intendedResult = intendedPrice.intoUint256();
console.log(intendedResult / 1e18);
UD60x18 firstPrice = target.getMarkPrice(100_000e18, 60_000e18, -100_000e18);
uint256 firstResult = firstPrice.intoUint256();
console.log(firstResult / 1e18);
UD60x18 secondPrice = target.getMarkPrice(50_000e18, 60_000e18, 0);
uint256 secondResult = secondPrice.intoUint256();
console.log(secondResult / 1e18);
}
We can see that depending on whose transaction gets processed first the fillPrice varies greatly.
Therefore implementing slippage checks is crucial.
Impact
Users will end up with inflated prices, thinking they might be make trades under favorable conditions, when it is the opposite.
Tools Used
Manual review
Recommendations
Consider having the option of having desired maxFillPrice so that users won't have to deal with bad surprises.
function createMarketOrder(CreateMarketOrderParams calldata params) external {
. . .
(
ctx.marginBalanceUsdX18,
ctx.requiredInitialMarginUsdX18,
ctx.requiredMaintenanceMarginUsdX18,
ctx.orderFeeUsdX18,
ctx.settlementFeeUsdX18,
+ ctx.fillPrice
) = simulateTrade({
tradingAccountId: params.tradingAccountId,
marketId: params.marketId,
settlementConfigurationId: SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta: params.sizeDelta
});
+ if(fillPrice > maxFillPrice) revert;
struct CreateMarketOrderParams {
uint128 tradingAccountId;
uint128 marketId;
int128 sizeDelta;
+ uint256 maxFillPrice;
}
Similarly before filling the market order in the SettlementBranch contract, the fillMarketOrder() function should make sure the calculated fillprice doesn't exceed the trader's expected fillPrice.
The same logic as in the fix above could be indeed applied.
