Neutra - audit

When a user accesses the `instantDeposit()` or `instantDepositGlp()` function of the `RouterV2` contract, assets are deposited into Neutra Finance and the user receives `snGlp` tokens as proof of their share, which can be used for identity verification when redeeming the assets. However, when the `_isStaked` parameter of the `instantWithdraw()` function is set to False, the user's identity is not verified when redeeming the assets, and the `_amount` parameter value is treated as the quantity of `nGlp` tokens. At this point, any user can call the `instantWithdraw()` function if there are `nGlp` tokens in the `RouterV2` contract, and receive the corresponding quantity of assets that other users have deposited into Neutra Finance, such as `DAI` or `fsGlp`. Based on this code logic, there is a risk of front-running attacks and potential loss of assets deposited by users in Neutra Finance.

The initial assets of the `StrategyVaultV2` contract of Neutra Finance are invested by users. Therefore, before the first user invests assets in Neutra Finance, the total assets in the `StrategyVaultV2` contract and the total issuance of the voucher token `nGlp` representing the aforementioned asset shares, are both zero. At this point, when the first user deposits assets and purchases GMX `Glp`, they need to mint a corresponding amount of `nGlp`. However, the `_mintNeutraGlp()` function does not handle the special case where the state variables `pendingStatus.totalSupplyBefore` and `pendingStatus.totalValueBefore` are both zero, resulting in the rollback of the first user's asset deposit operation.

The function `gmxPositionCallback` can be called by anyone as it has no access restriction. This enables anyone to call this and control the position execution result. The function `confirmCallback()` can be called by anyone as it has no access restriction.

One or more logic contracts do not protect their initializers. An attacker can call the initializer and assume ownership of the logic contract, whereby she can perform privileged operations that either indirectly break the proxy by destroying the logic contract or trick unsuspecting users into believing that she is the owner of the upgradeable contract.

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. ### External call(s) ```solidity=219 IRewardTracker(feeNeuGlpTracker).stakeForAccount(address(this), msg.sender, nGlp, _amount); ``` ```solidity=220 IRewardTracker(stakedNeuGlpTracker).stakeForAccount(msg.sender, msg.sender, feeNeuGlpTracker, _amount); ``` ### State variables written after the call(s) ```solidity=223 withdrawRound[msg.sender] = 0; ``` --- ### External call(s) ```solidity=322 uint256 amountOut = IRouter(router).confirmAndBuy(pendingDealAmount, address(this)); ``` ```solidity=324 _updateRewards(); ``` * This function call executes the following external call(s). * In `BatchRouter._updateRewards`, * `esNeuAmount = IRewardTracker(stakedNeuGlpTracker).claimForAccount(address(this),address(this))` * In `BatchRouter._updateRewards`, * `wantAmount = IRewardTracker(feeNeuGlpTracker).claimForAccount(address(this),address(this))` ### State variables written after the call(s) ```solidity=334 pendingDealAmount = 0; ``` ```solidity=331 totalSnGlpReceivedAmount += amountOut; ``` --- ### External call(s) ```solidity=322 uint256 amountOut = IRouter(router).confirmAndBuy(pendingDealAmount, address(this)); ``` ```solidity=324 _updateRewards(); ``` * This function call executes the following external call(s). * In `BatchRouter._updateRewards`, * `esNeuAmount = IRewardTracker(stakedNeuGlpTracker).claimForAccount(address(this),address(this))` * In `BatchRouter._updateRewards`, * `wantAmount = IRewardTracker(feeNeuGlpTracker).claimForAccount(address(this),address(this))` ```solidity=337 uint256 amountOut = IRouter(router).confirmAndSell(pendingDealAmount, address(this)); ``` ### State variables written after the call(s) ```solidity=345 executed = false; ``` --- ### External call(s) ```solidity=337 uint256 amountOut = IRouter(router).confirmAndSell(pendingDealAmount, address(this)); ``` ### State variables written after the call(s) ```solidity=342 pendingDealAmount = 0; ``` --- ### External call(s) ```solidity=149 IStrategyVault(strategyVault).harvest(); ``` ```solidity=151 _checkLastFundingTime(); ``` * This function call executes the following external call(s). * In `RouterV2._checkLastFundingTime`, * `IVault(gmxVault).updateCumulativeFundingRate(want)` ### State variables written after the call(s) ```solidity=152 _setPendingStatus(true); ``` * This function call executes the following assignment(s). * In `RouterV2._setPendingStatus`, * `pendingStatus.isProgress = true` * In `RouterV2._setPendingStatus`, * `pendingStatus.recipient = msg.sender` * In `RouterV2._setPendingStatus`, * `pendingStatus.totalValueBefore = IStrategyVault(strategyVault).totalValue()` * In `RouterV2._setPendingStatus`, * `pendingStatus.totalSupplyBefore = IERC20(nGlp).totalSupply()` * In `RouterV2._setPendingStatus`, * `pendingStatus.longValueBefore = _gmxHelper.getLongValue(IERC20(fsGlp).balanceOf(strategyVault))` * In `RouterV2._setPendingStatus`, * `pendingStatus.wbtcCollateralBefore = wbtcCollateral` * In `RouterV2._setPendingStatus`, * `pendingStatus.wethCollateralBefore = wethCollateral` --- ### External call(s) ```solidity=192 IStrategyVault(strategyVault).harvest(); ``` ```solidity=194 _checkLastFundingTime(); ``` * This function call executes the following external call(s). * In `RouterV2._checkLastFundingTime`, * `IVault(gmxVault).updateCumulativeFundingRate(want)` ### State variables written after the call(s) ```solidity=195 _setPendingStatus(true); ``` * This function call executes the following assignment(s). * In `RouterV2._setPendingStatus`, * `pendingStatus.isProgress = true` * In `RouterV2._setPendingStatus`, * `pendingStatus.recipient = msg.sender` * In `RouterV2._setPendingStatus`, * `pendingStatus.totalValueBefore = IStrategyVault(strategyVault).totalValue()` * In `RouterV2._setPendingStatus`, * `pendingStatus.totalSupplyBefore = IERC20(nGlp).totalSupply()` * In `RouterV2._setPendingStatus`, * `pendingStatus.longValueBefore = _gmxHelper.getLongValue(IERC20(fsGlp).balanceOf(strategyVault))` * In `RouterV2._setPendingStatus`, * `pendingStatus.wbtcCollateralBefore = wbtcCollateral` * In `RouterV2._setPendingStatus`, * `pendingStatus.wethCollateralBefore = wethCollateral` --- ### External call(s) ```solidity=247 IStrategyVault(strategyVault).harvest(); ``` ```solidity=249 _checkLastFundingTime(); ``` * This function call executes the following externa

The function `handleFailure` is used to handle the failure once the create position fails. However, it does not reset the position execution status to `PositionExecutionStatus.NONE`. ```solidity=41 function _successCallback(bool _isIncrease, bytes32 _requestKey) internal { if (status == PositionExecutionStatus.NONE) { status = PositionExecutionStatus.PARTIAL; IRouter(router).firstCallback(_isIncrease, _requestKey); } else if (status == PositionExecutionStatus.PARTIAL) { status = PositionExecutionStatus.NONE; IRouter(router).secondCallback(_isIncrease, _requestKey); } } ``` According to the logic above, the `PositionExecutionStatus` controls the order in which callback functions are run. If the second asset of a position is executed failure by the keeper of the GMX and the position execution status is not reset to `PositionExecutionStatus.NONE`, in the next deposit or withdrawal, the second callback function `secondCallback` will be first called. This could potentially cause the user to not receive the `snGlp` share when depositing or to not be able to withdraw the `want` asset.

When accessing the functions `instantDeposit()`, `instantDepositGlp()`, and `instantWithdraw()` in the Neutra `RouterV2` contract for staking or redemption operations, if the cumulative funding rate for the `GMX` contract's staked tokens, specifically DAI, has not been updated for over an hour, the `updateCumulativeFundingRate()` function in the `GMX Vault` contract will be called to update it. In the `updateCumulativeFundingRate()` function, the cumulative funding rate for the staked token, DAI, is also checked to see if it has not been updated for a certain duration. This duration is given by the state variable `fundingInterval`, which is set to one hour in the `Vault` contract on the chain, but can be modified to be greater than one hour by calling the `setFundingRate()` function. However, according to the logic in the `_checkLastFundingTime()` function in the Neutra `RouterV2` contract, the cumulative funding rate for the staked token, DAI, needs to be updated every hour. If the `fundingInterval` state variable is modified to be greater than one hour, the aforementioned cumulative funding rate will not be updated every hour according to the logic in `_checkLastFundingTime()`.

There is no validation ensure that the function only can be executed if the `confirmed` is false.

The code in lines 387 and 388 is used to calculate the proportion of WBTC and WETH in the GLP pool. Based on the calculated proportion, asset allocations for purchasing GLP and shorting WBTC and WETH are determined. The proportion calculation formula divides the assets of WBTC/WETH in the pool by the total assets in the pool. However, the calculation of these two assets is inconsistent. When calculating the total assets in the pool, the loss incurred by users shorting is taken into account, whereas when calculating the assets of WBTC/WETH, this is not considered. This results in the proportion of WBTC/WETH being calculated as smaller than the actual value.

According to the logic of the `RouterV2` contract code by Neutra, users are responsible for paying the execution fees in ETH for opening or closing positions in GMX. The payable functions that involve opening or closing positions, namely `instantDeposit()`, `instantDepositGlp()`, and `instantWithdraw()`, all check whether the ETH transferred by the user is sufficient to cover the execution fees. However, the conditional statement in the code is `msg.value >= execution * n`. As a result, if the amount of ETH transferred by the user is greater than the required execution fees, there will be a surplus after deducting the GMX execution fees. In line 301 of the `instantWithdraw()` function, the excess ETH is returned to the user. However, there is no code logic to return excess ETH in the `instantDeposit()` and `instantDepositGlp()` functions. This design is unreasonable.