Summary
Incorrect price impact calculation of the VaultReader::getPriceImpactInCollateral function
Vulnerability Details
If the trade improves the long/short balance or tokens in the GMX pool then there would be a positive price impact, otherwise there would be a negative price impact.
The example opening GMX positions which have two side of price impact:
If opening a long position with a positive price impact, the position's entry price would be lower. A negative price impact would result in a entry price would be lower, the user will get more shares for the same amount of collateral.
If opening a long position with a negative price impact, the position's entry price would be higher, the user will get less shares for the same amount of collateral.
The incorrect is presented in VaultReader::getPriceImpactInCollateral function, which calculate by expectedSizeInTokensDelta.toInt256() - realSizeInTokensDelta.toInt256() instead of realSizeInTokensDelta.toInt256() - expectedSizeInTokensDelta.toInt256(). This make the the flip side of the price impact. However, the PerpetualVault::afterOrderExecution function is calculate in the flip side, which result the increased is correct unintentionally.
function getPriceImpactInCollateral(
bytes32 positionKey,
uint256 sizeDeltaInUsd,
uint256 prevSizeInTokens,
MarketPrices memory prices
) external view returns (int256) {
uint256 expectedSizeInTokensDelta = sizeDeltaInUsd / prices.indexTokenPrice.min;
uint256 curSizeInTokens = getPositionSizeInTokens(positionKey);
uint256 realSizeInTokensDelta = curSizeInTokens - prevSizeInTokens;
@> int256 priceImpactInTokens = expectedSizeInTokensDelta.toInt256() - realSizeInTokensDelta.toInt256();
int256 priceImpactInCollateralTokens = priceImpactInTokens * prices.indexTokenPrice.min.toInt256() / prices.shortTokenPrice.min.toInt256();
return priceImpactInCollateralTokens;
}
function afterOrderExecution(
bytes32 requestKey,
bytes32 positionKey,
IGmxProxy.OrderResultData memory orderResultData,
MarketPrices memory prices
) external nonReentrant {
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}
_gmxLock = false;
if (orderResultData.isSettle) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
emit GmxPositionCallbackCalled(requestKey, true);
return;
}
if (orderResultData.orderType == Order.OrderType.MarketIncrease) {
curPositionKey = positionKey;
if (flow == FLOW.DEPOSIT) {
uint256 amount = depositInfo[counter].amount;
uint256 feeAmount = vaultReader.getPositionFeeUsd(market, orderResultData.sizeDeltaUsd, false) / prices.shortTokenPrice.min;
uint256 prevSizeInTokens = flowData;
@> int256 priceImpact = vaultReader.getPriceImpactInCollateral(curPositionKey, orderResultData.sizeDeltaUsd, prevSizeInTokens, prices);
@> uint256 increased;
@> if (priceImpact > 0) {
@> increased = amount - feeAmount - uint256(priceImpact) - 1;
@> } else {
@> increased = amount - feeAmount + uint256(-priceImpact) - 1;
@> }
@> _mint(counter, increased, false, prices);
nextAction.selector = NextActionSelector.FINALIZE;
} else {
_updateState(false, orderResultData.isLong);
}
...
Impact
The VaultReader::getPriceImpactInCollateral is external function, this may provide the incorrect price impact to frontend and other contracts.
Moreover, although the issue is not directly affecting the security of the protocol, it may lead to confusion and affect the maintainability of the protocol.
PoC
Insert import "@openzeppelin/contracts/utils/math/SafeCast.sol"; to the test/PerpetualVault.t.sol file
Use the SafeCast library in the PerpetualVaultTest contract of test/PerpetualVault.t.sol file
contract PerpetualVaultTest is Test, ArbitrumTest {
+ using SafeCast for uint256;
....
Copy the following test case to the test/PerpetualVault.t.sol file
Run tests with forge test --mt test_Incorrect_PriceImpact_Calculation --rpc-url arbitrum
function test_Incorrect_PriceImpact_Calculation() external {
address keeper = PerpetualVault(vault2x).keeper();
address alice = makeAddr("alice");
depositFixtureInto2x(alice, 1e10);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3390000000000000);
vm.prank(keeper);
PerpetualVault(vault2x).run(true, true, prices, data);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
deal(alice, 1 ether);
depositFixtureInto2x(alice, 1e10);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
bytes32 curPositionKey = PerpetualVault(vault2x).curPositionKey();
uint256 sizeDeltaInUsd = 20000593429804817500000000000000000;
uint256 prevSizeInTokens = 5910828393414802529;
MarketPrices memory marketPrices;
marketPrices.indexTokenPrice = PriceProps({
min: 3386174003773116,
max: 3386174003773116
});
marketPrices.longTokenPrice = PriceProps({
min: 3386174003773116,
max: 3386174003773116
});
marketPrices.shortTokenPrice = PriceProps({
min: 1000019671490240875000000,
max: 1000019671490240875000000
});
address gmxProxy = address(PerpetualVault(vault2x).gmxProxy());
(bytes32 requestKey, ) = GmxProxy(payable(gmxProxy)).queue();
MockData.OracleSetPriceParams memory params = mockData.getOracleParams();
address gmxKeeper = address(0x6A2B3A13be0c723674BCfd722d4e133b3f356e05);
address orderHandler = address(0xe68CAAACdf6439628DFD2fe624847602991A31eB);
vm.expectCall(address(PerpetualVault(vault2x).vaultReader()),
abi.encodeWithSelector(VaultReader.getPriceImpactInCollateral.selector, curPositionKey, sizeDeltaInUsd, prevSizeInTokens, marketPrices));
vm.prank(gmxKeeper);
IOrderHandler(orderHandler).executeOrder(requestKey, params);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
int256 actualPriceImpact = reader.getPriceImpactInCollateral(curPositionKey, sizeDeltaInUsd, prevSizeInTokens, marketPrices);
uint256 expectedSizeInTokensDelta = sizeDeltaInUsd / marketPrices.indexTokenPrice.min;
uint256 curSizeInTokens = reader.getPositionSizeInTokens(curPositionKey);
uint256 realSizeInTokensDelta = curSizeInTokens - prevSizeInTokens;
int256 priceImpactInTokens = realSizeInTokensDelta.toInt256() - expectedSizeInTokensDelta.toInt256();
int256 expectedPriceImpact = priceImpactInTokens * marketPrices.indexTokenPrice.min.toInt256() / marketPrices.shortTokenPrice.min.toInt256();
vm.assertTrue(actualPriceImpact < 0);
vm.assertTrue(expectedPriceImpact >= 0);
}
Tools Used
Manual Review
Recommendations
Change the logic of VaultReader::getPriceImpactInCollateral function to calculate the correctprice impact by use realSizeInTokensDelta.toInt256() - expectedSizeInTokensDelta.toInt256() instead, and then adjust the condition of the PerpetualVault::afterOrderExecution to align with the change.
function getPriceImpactInCollateral(
bytes32 positionKey,
uint256 sizeDeltaInUsd,
uint256 prevSizeInTokens,
MarketPrices memory prices
) external view returns (int256) {
uint256 expectedSizeInTokensDelta = sizeDeltaInUsd / prices.indexTokenPrice.min;
uint256 curSizeInTokens = getPositionSizeInTokens(positionKey);
uint256 realSizeInTokensDelta = curSizeInTokens - prevSizeInTokens;
- int256 priceImpactInTokens = expectedSizeInTokensDelta.toInt256() - realSizeInTokensDelta.toInt256();
+ int256 priceImpactInTokens = realSizeInTokensDelta.toInt256() - expectedSizeInTokensDelta.toInt256();
int256 priceImpactInCollateralTokens = priceImpactInTokens * prices.indexTokenPrice.min.toInt256() / prices.shortTokenPrice.min.toInt256();
return priceImpactInCollateralTokens;
}
function afterOrderExecution(
bytes32 requestKey,
bytes32 positionKey,
IGmxProxy.OrderResultData memory orderResultData,
MarketPrices memory prices
) external nonReentrant {
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}
// MarketPrices memory marketPrices = gmxProxy.getMarketPrices(market);
_gmxLock = false;
// If the current action is `settle`
if (orderResultData.isSettle) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
emit GmxPositionCallbackCalled(requestKey, true);
return;
}
if (orderResultData.orderType == Order.OrderType.MarketIncrease) {
curPositionKey = positionKey;
if (flow == FLOW.DEPOSIT) {
uint256 amount = depositInfo[counter].amount;
uint256 feeAmount = vaultReader.getPositionFeeUsd(market, orderResultData.sizeDeltaUsd, false) / prices.shortTokenPrice.min;
uint256 prevSizeInTokens = flowData;
int256 priceImpact = vaultReader.getPriceImpactInCollateral(curPositionKey, orderResultData.sizeDeltaUsd, prevSizeInTokens, prices);
- uint256 increased;
- if (priceImpact > 0) {
- increased = amount - feeAmount - uint256(priceImpact) - 1;
- } else {
- increased = amount - feeAmount + uint256(-priceImpact) - 1;
- }
- _mint(counter, increased, false, prices);
+ uint256 amountWithImpact;
+ if (priceImpact >= 0) {
+ amountWithImpact = amount - feeAmount + uint256(priceImpact) - 1;
+ } else {
+ amountWithImpact = amount - feeAmount - uint256(-priceImpact) - 1;
+ }
+ _mint(counter, amountWithImpact, false, prices);
nextAction.selector = NextActionSelector.FINALIZE;
} else {
_updateState(false, orderResultData.isLong);
}
...
