Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

VaultReader's returns incorrect status `isLong` as well doing Incorrect Collateral Check

dingo2077

VaultReader's getPositionInfo() returns incorrect status isLong when no open positions exist.

Summary

The function getPositionInfo() in VaultReader.sol contract by default sets isLong to true when there no open position with associated key (sizeInTokens = 0);
This value not reflects the actual status of position, potentially leading to data inconsistencies in UI representations and downstream applications.

Vulnerability Details

In the getPositionInfo function, when sizeInTokens == 0, the function returns a PositionData struct with default values:

if (sizeInTokens == 0) {
return PositionData({
sizeInUsd: 0,
sizeInTokens: 0,
collateralAmount: 0,
netValue: 0,
pnl: 0,
isLong: true // set by default
});
}

The isLong flag is hardcoded to true without any consideration of the position's actual direction.

Also these functions: getPositionInfo(), getNegativeFundingFeeAmount(), getPnl() have incorrect naming of variable which handle returned value from getPositionSizeInUsd():

...
uint256 sizeInTokens = getPositionSizeInUsd(key);
...

It is essential to use sizeInUsd. Although it does not involve any calculations, using sizeInUsd is the preferred approach.

Impact

-UI/UX confusion for users viewing closed position details;
-Potential issues in integrated protocols that rely on function getPositionInfo();

Tools Used

Manual code review

Recommendations

There are several possible solutions to improve this behavior:

  1. Remove the hardcoded True and set's isLong to false:

if (sizeInTokens == 0) {
return PositionData({
sizeInUsd: 0,
sizeInTokens: 0,
collateralAmount: 0,
netValue: 0,
pnl: 0,
isLong: false // Use false as neutral indicator
});
}

  1. Add a new field bool isActive to the new structure to explicitly indicate that the position is inactive:

struct PositionData {
uint256 sizeInUsd;
uint256 sizeInTokens;
uint256 collateralAmount;
uint256 netValue;
int256 pnl;
bool isLong;
bool isActive; // If getPositionSizeInUsd == 0 return false
}

Incorrect Collateral Check for Closed Positions at VaultReader contract.

Summary

The function willPositionCollateralBeInsufficient() in VaultReader incorrectly returns true (indicating insufficient collateral) when called for positions that no longer exist (sizeInUsd == 0). This creates misleading signals for users/offchain keeper attempting to add more collateral with closed positions. Even if keeper has other health check logic for open positions, users/their contracts still can receives incorrect result.

Vulnerability Details

function willPositionCollateralBeInsufficient(...) external view returns (bool) {
if (getPositionSizeInUsd(positionKey) == 0) return true; // Incorrect check
// ... rest of logic ...
}

The function unconditionally returns true if the position has sizeInUsd == 0, regardless of the actual collateral state. This is fundamentally incorrect because closed positions (size = 0) cannot have insufficient collateral by definition (collateral is removed when positions close).

Also these functions: getPositionInfo(), getNegativeFundingFeeAmount(), getPnl() have incorrect naming of variable which handle returned value from getPositionSizeInUsd():

...
uint256 sizeInTokens = getPositionSizeInUsd(key);
...

It is essential to use sizeInUsd. Although it does not involve any calculations, using sizeInUsd is the preferred approach.

Impact

Users/contracts calling this function for closed positions (e.g., to check if they can re-open a position) will receive a false "insufficient collateral" signal. This could lead to unnecessary actions like depositing extra collateral or abandoning valid operations depending of how Gamma offchain script reacts.

Tools Used

Manual code analysis

Recommendations

if (getPositionSizeInUsd(positionKey) == 0) return false; //consider return False. No positions -> no requirements.

Also another approach is to return a structure that explicitly indicates whether the position is closed.

struct willPositionCollateralBeInsufficientResult {
bool willBeSufficient;
bool isActive; // If getPositionSizeInUsd == 0, return false
}

VaultReader: getPositionInfo() returns isLong always False

Summary

A critical inconsistency has been identified in the VaultReader contract where the getPositionInfo() function consistently returns false for the isLong parameter, even when the position is actually long. This contradicts the data returned by direct calls to GMXReader with identical input parameters, which correctly returns true for long positions.

Vulnerability Details

The VaultReader.getPositionInfo() function incorrectly reports position sides (long/short) despite using the same underlying GMXReader contract and dataStore:

​
function getPositionInfo(bytes32 key, MarketPrices memory prices) external view returns (PositionData memory) {
...
PositionInfo memory positionInfo = gmxReader.getPositionInfo(
address(dataStore),
referralStorage,
key,
prices,
uint256(0),
address(0),
true
);
// ... other calculations ...
​
return PositionData({
// ... other fields ...
isLong: positionInfo.position.flags.isLong // Always returns false
});
}

Foundry test:
//just add to PerpetualVault.t.sol:

function test_ReturnIsLongFalse() external {
address keeper = PerpetualVault(vault2x).keeper();
address alice = makeAddr("alice");
depositFixtureInto2x(alice, 1e8);
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);
PerpetualVault.FLOW flow = PerpetualVault(vault2x).flow();
assertEq(uint8(flow), 2);
assertEq(PerpetualVault(vault2x).positionIsClosed(), true);
(PerpetualVault.NextActionSelector selector, ) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector), 0);
// assertEq(uint8(PerpetualVault(vault).isNextAction()), 0);
​
GmxOrderExecuted2x(true);
bytes32 curPositionKey = PerpetualVault(vault2x).curPositionKey();
assertTrue(curPositionKey != bytes32(0));
assertEq(PerpetualVault(vault2x).beenLong(), true);
​
VaultReader.PositionData memory positionData = reader.getPositionInfo(curPositionKey, prices);
console.log("positionData.isLong: ", positionData.isLong);
console.log("positionData.sizeInUsd ", positionData.sizeInUsd);
}

But direct calls to GMXReader.getPositionInfo() with identical parameters correctly return true for long positions.

Impact

Misleading position information for users and integrating protocols as well gamma off-chain script.

Tools Used

Manual review

Recommendations

Consider to add direct validation of position side: bool isLongPosition = Position.isLong(dataStore, key);

Updates

Lead Judging Commences

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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