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

The `prices` variable in the `PerpetualVault::deposit()` function isn't initialised with data before being passed to the `PerpetualVault::_mint()` function, resulting in inaccurate and erroneous minting.

web3angel

Summary

The prices variable in the PerpetualVault::deposit() function is declared as MarketPrices memory prices in the code but is not initialised with any values. This "uninitialised" variable is then passed into the PerpetualVault::_mint() function.

Vulnerability Details

Extract of the PerpetualVault::deposit() function:

function deposit(uint256 amount) external payable nonReentrant {
_noneFlow();
if (depositPaused == true) {
revert Error.Paused();
}
if (amount < minDepositAmount) {
revert Error.InsufficientAmount();
}
if (totalDepositAmount + amount > maxDepositAmount) {
revert Error.ExceedMaxDepositCap();
}
flow = FLOW.DEPOSIT;
collateralToken.safeTransferFrom(msg.sender, address(this), amount);
counter++;
depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0));
totalDepositAmount += amount;
EnumerableSet.add(userDeposits[msg.sender], counter);
​
if (positionIsClosed) {
@> MarketPrices memory prices;
@> _mint(counter, amount, false, prices);
_finalize(hex"");
} else {
_payExecutionFee(counter, true);
// mint share token in the NextAction to involve off-chain price data and improve security
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}
}

Prices will most likely contain 0 as a value, as it is represented by uint256 as per the declarations below extracted from Libraries::StructData.sol.

struct PriceProps {
uint256 min;
uint256 max;
}
struct MarketPrices {
PriceProps indexTokenPrice;
PriceProps longTokenPrice;
PriceProps shortTokenPrice;
}

The PerpetualVault::_mint() function then uses prices to calculate the totalAmountBefore.

function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
uint256 _shares;
if (totalShares == 0) {
_shares = depositInfo[depositId].amount * 1e8;
} else {
uint256 totalAmountBefore;
if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
} else {
@> totalAmountBefore = _totalAmount(prices) - amount;
}
if (totalAmountBefore == 0) totalAmountBefore = 1;
_shares = amount * totalShares / totalAmountBefore;
}
​
depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;

Impact

Not using actual price data can result in incorrect share calculations for the user and the user getting less funds than they should when they withdraw their funds from the protocol.
The deposit, mint and withdraw functions are critical functions to the proper functioning of the protocol and are used by each user as they either start using the protocol or want to get their funds back.

Tools Used

Manual review

Recommendations

Ensure that the prices variable is initialised with the most up to date, accurate and appropriate market data. This can be done either by fetching the market prices through the GmxProxy::getMarketPrices() function or by directly populating the prices struc with real-time market data before passing it to mint. The preferred approach would be to use GmxProxy::getMarketPrices() as per below:

Extract from PerpetualVault::deposit()

if (positionIsClosed) {
- MarketPrices memory prices;
+ MarketPrices memory marketPrices = gmxProxy.getMarketPrices(market);
_mint(counter, amount, false, prices);
_finalize(hex"");

There is another instance of this occurring in the PerpetualVault::withdraw() function as per the code extract below:

function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
_noneFlow();
flow = FLOW.WITHDRAW;
flowData = depositId;
​
if (recipient == address(0)) {
revert Error.ZeroValue();
}
if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {
revert Error.Locked();
}
if (EnumerableSet.contains(userDeposits[msg.sender], depositId) == false) {
revert Error.InvalidUser();
}
if (depositInfo[depositId].shares == 0) {
revert Error.ZeroValue();
}
​
depositInfo[depositId].recipient = recipient;
_payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle(); // Settles any outstanding fees and updates state before processing withdrawal
} else {
@> MarketPrices memory prices;
_withdraw(depositId, hex"", prices);
}
}

The same mitigation would apply for this instance.

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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