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.
Attacker deposits a minimal amount , this can be the minimum deposit and gets shares
Attacker deposits collateral token amount directly to the vault
Unsuspecting user deposits funds and gets back 0 shares
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 {
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;
@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;
}
if (totalAmountBefore == 0) totalAmountBefore = 1;
@audit>> _shares = amount * totalShares / totalAmountBefore;
}
@audit>> depositInfo[depositId].shares = _shares;
@audit>> totalShares = totalShares + _shares;
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[counter].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
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) {
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