DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Vault inflation attack possible as attacker can inflate the total amount to steal another users deposited funds

Summary

Though this contract is not a ERC4626 vault an attacker can still perform the first depositor's attack and effectively steal an unsuspecting users fund.

Vulnerability Details

This occurs when the position is closed and the vault is new.

  1. Attacker deposits a minimal amount , this can be the minimum deposit and gets shares

  2. Attacker deposits collateral token amount directly to the vault

  3. Unsuspecting user deposits funds and gets back 0 shares

  4. Attacker withdraws users token as if they are the profit made from the trades while the user gets nothing

function deposit(uint256 amount) external nonReentrant payable { // marked as payable but do we expect ETH????
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; // calculate shares based on this and scale up ...... amount deposited/total amount/ now i understand why we use address this
EnumerableSet.add(userDeposits[msg.sender], counter);
if (positionIsClosed) { // if user passes in msg.value they will lose it and it will not be refunded ..... try something else
MarketPrices memory prices;
@audit>> _mint(counter, amount, false, prices);
_finalize(hex'');
}
/**
* @notice this function is an end of deposit flow.
* @dev should update all necessary global state variables
*
* @param depositId `depositId` of mint operation
* @param amount actual deposit amount. if `_isLongOneLeverage` is `true`, amount of `indexToken`, or amount of `collateralToken`
*/
function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
uint256 _shares;
@audit>> if (totalShares == 0) {
@audit>> _shares = depositInfo[depositId].amount * 1e8;
} else {
uint256 totalAmountBefore;
if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
} else {
@audit>> totalAmountBefore = _totalAmount(prices) - amount; // use address this balance
}
if (totalAmountBefore == 0) totalAmountBefore = 1; // this is nice
@audit>> _shares = amount * totalShares / totalAmountBefore; // no check to prevent shares from being 0. honest profit first depositor attack
}
@audit>> depositInfo[depositId].shares = _shares; // inflation attack High loss of fund
@audit>> totalShares = totalShares + _shares;
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[counter].executionFee > usedFee) { // USED E // are we always fulfilling the last counter // use depositid instead we might be using somethings else here
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {} // if this fails where is the refunded money going to
}
}
emit Minted(depositId, depositInfo[depositId].owner, _shares, amount);
}

The unsuspecting users will lose their funds

/**
* @notice Withdraws collateral of user.
* @dev If position is GMX position, first call `settle` function to settle pending fees
* in order to avoid charging all fees from the withdrawer.
* @param recipient The address to receive the tokens.
* @param depositId The deposit ID to withdraw.
*/
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();
}
@audit>> if (depositInfo[depositId].shares == 0) { // for the inflation attack
revert Error.ZeroValue();
}

Impact

First depositor attack can be carried out in the contract causing users to lose their assets.

Tools Used

Manual Review

Recommendations

Nest a check to ensure that the shares calculated is not zero, also we can increase the scaling factor to the total shares from 1e8 to 1e18 making the attack overly expensive for an attacker

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

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