GLP to GMX V2 Migration

In the _redeemGlp function, users are allowed to make any arbitrary external calls with the redemptionInfo.externalCallTargets and redemptionInfo.externalCallDataList.

Therefore a user seeking to redeem GLP using the discounted redemption may do so as the external call is executed within the context of the withReducedRedemptionFees modifier.

Users may abuse the system in a similar way by simply providing an EOA address as the redemptionInfo.receiver rather than the DepositVault, or by using the subsequent external call to transfer out the redeemed tokens to their EOA.

Require that the redeemedTokenAmount (or at least a majority, accounting for potential fees & slippage) end up in the DepositVault contract.

This validation also serves as a safety net in the event that the provided redemptionInfo.externalCallTargets or redemptionInfo.externalCallDataList hold errors.

GMX Team: Acknowledged and comment added in commit 2de90ca.

Currently GLP consists of a large amount of USDC.e (bridged USDC), which has a different token address than USDC. On the other hand, GMX V2 pools all use USDC.

Because a user is required to pass a migrationItem.short.token that matches the cache.market.shortToken, they cannot redeem with USDC.e as tokenOut since the short token validation will revert.

Rather, the user is forced to redeem for the limited amount of USDC directly so that they can deposit the USDC into the GMX V2 market.

In the case of USDC, modify the InvalidShortTokenForMigration check such that USDC.e can still pass and then be swapped for native USDC.

Furthermore, consider adding a state check after the external calls to check the token balances of the depositVault, to ensure the user has not mistakenly sent the USDC.e to the GMX V2 system and lost it.

GMX Team: The InvalidLongTokenForMigration and InvalidShortTokenForMigration checks have been removed in commit 3a696d5.

If the provided msg.value is greater than the executionFee * migrationItems.length then the excess Ether is not refunded to the user and can be used by the user who calls the migrate function next.

Add validation to ensure that msg.value == executionFee * migrationItems.length, otherwise refund any excess Ether to the caller.

GMX Team: The recommendation has been implemented in commit 58312d4.

In RewardRouter.sol for GMX V1, function unstakeAndRedeemGlp() requires that _glpAmount > 0:

require(_glpAmount > 0, "RewardRouter: invalid _glpAmount");

In the GlpMigrator contract, if a user wants to only redeem for a the long token in a market, and leaves the GlpRedemption short with migrationItem.short.glpAmount = 0, the migration will fail due to the above revert.

This is unexpected behavior as it forces users to not only populate the migrationItem.short.token to match the market's short token, but also set a miniscule amount of glpAmount to redeem for the short token to avoid migration failure.

The same behavior applies if a user wants to solely redeem for the short token in a market, and leave the long token untouched.

If the glpAmount for either the long or short token is 0, skip the call unstakeAndRedeemGlp() for that token.

GMX Team: The recommendation has been implemented in commit 58312d4.

GMX is choosing to reduce the burn fee to further incentivize migration from GMX V1 to its latest GMX V2 system. However, there is no guarantee that the reducedMintBurnFeeBasisPoints is less than or equal to the current mintBurnFeeBasisPoints.

The burn fee can be increased to be larger than its current value, causing users to redeem less tokens than expected.

Inside modifier withReducedRedemptionFees, only update the burn fee in GMX V1 if the _reducedMintBurnFeeBasisPoints is smaller:

bool shouldUpdateFees = _reducedMintBurnFeeBasisPoints < mintBurnFeeBasisPoints;

GMX Team: The recommendation has been implemented in commit 58312d4.

The externalCallTargets and externalCallDataList are used to call an external protocol with user-passed data.

However, externalCallDataList may have a different length than externalCallTargets, potentially causing an out-of-bounds error or the incorrect data being used for a particular target.

Similarly, refundTokens and refundReceivers may be different lengths, potentially causing an out-of-bounds error or funds being sent to an unintended receiver.

Add validation such that externalCallTargets and externalCallDataList are the same length and that refundTokens and refundReceivers are the same length:

require(externalCallTargets.length == externalCallDataList.length)

require(refundTokens.length == refundReceivers.length)

GMX Team: The recommendation has been implemented in commit 3a696d5.

The low-level call returns a success boolean of true if the target contract does not exist. As a result, the migration may not detect some failed external calls, leading to loss of funds for users.

Consider implementing a contract existence check prior to the call.

GMX Team: The recommendation has been implemented in commit 3a696d5.

In the makeExternalCalls function, the arbitrary refundToken is transferred using the transfer function, however safeTransfer should be used to avoid potential loss if the token chooses to return false rather than reverting upon failure.

Prefer safeTransfer to transfer.

GMX Team: The recommendation has been implemented in commit 3a696d5.

The same executionFee is used for every migrationItem in the migrationItems list, however some migrations may require a smaller executionFee than others depending on if they are single token deposits.

Allow an individual executionFee to be specified on a migrationItem basis.

GMX Team: The recommendation has been implemented in commit 58312d4.

In order for glpTimelock.setSwapFees() to succeed, the GlpMigrator contract must be given the necessary access control to bypass the onlyKeeperAndAbove modifier in GMX V1.

Set the migration contract as a handler in the GLP Timelock contract.

GMX Team: Confirmed the GlpMigrator will have the necessary privileges.