MagicGLP Upgrade

The last claimer from the MagicGLP system can manipulate the oracle to report a higher price per share than is really available for the underlying MagicLP collateral shares. This could be used to create bad debt as the final claimer who manipulates the price in this way can end up borrowing more MIM than their underlying shares are actually worth. Consider the following example:

• Claimer A is the final claimer of tokens from the MagicLP upgraded contract
• The totalSupply is 10e18 tokens
• The total balance of the MagicLP contract is 10 GM tokens (assume GM is the only claim token for simplicity)
• Claimer A claims all but 15 wei of their shares, e.g. claim(10e18 - 15)
• Claimer A receives 99.9% of their claimed token amounts, leaving only some dust tokens in the vault
• The price of Magic LP is consistent in this action
• Now Claimer A maliciously sends $10 worth of GM tokens directly to the MagicLP contract
• Notice that these funds are technically still under custody of Claimer A since they are the only holder of vault tokens
• Now the state of MagicLP is, totalSupply = 15 wei, TVL = $10
• The MGLPV2Oracle reports the price as: magicGlp.totalSupply() * 1e36 / (tvl * 10**magicGlp.decimals()) => 15 * 1e36 / (10e18 * 1e18) = 1
• This is the inverse of price, measured as shares per asset. Therefore the value of shares reported by the oracle has been inflated due to the truncation.
• With this price inflation the users 15 wei of collateral is valued at $15 in the CauldronV4 contract, meanwhile it can only be redeemed for $10 of TVL from the underlying vault This attack path could be scaled up with a bigger donation, allowing more MIM to be borrowed than can be recovered from the vault.

Consider rounding the resulting price up in the MGLPV2Oracle.

In the claim function when a token claim has zero value to be claimed the transfer is still attempted to the user. However for some ERC20 implementations a zero value transfer reverts. This would DoS the claim action for users when a supported claim token has no balance in the contract or very small balance which rounds down to 0 during the claim calculations.

Only perform the token transfer if the amount being transferred is nonzer.

The implementation of ERC4626 used by the MagicGLP contract relies on the totalAssets function to determine the result of the convertToShares, convertToAssets, previewDeposit, previewMint, previewWithdraw, maxWithdraw, and previewRedeem functions. The RevertingToken contract is intended to disable the ERC4626 behavior, and the write functions from the ERC4626 contract have indeed been successfully disabled. However these view functions are not totally disabled and exhibit unexpected behavior because the totalAssets function does not revert. The totalAssets function is intended to report how much underlying assets are available in the 4626 vault. And the RevertingToken has implemented the balanceOf function to allow deal() to be used in testing. As a result, rather than reverting, the totalAssets function returns an empty balanceOf result for the vault address. This results in unexpected results from the mentioned view functions, ranging from divide by zero panic reverts to successful executions with zero value results.

Be aware of this unexpected behavior from these view functions and ensure that users do not rely on them to determine how much they can claim / redeem.

If a callback claim token is supported it can be leveraged to exploit the system because the attacker can gain control of the transaction after the shares have been burned but before subsequent claim token transfers have been processed in the claim function. This will result in a stepwise decrease in the price of the vault shares occurring after the malicious actors influence over the transaction passes, thus allowing them to open a loan at a higher exchange rate before it drops right afterwards.

Be aware of this exploit for tokens with transfer callbacks and be sure to not support such tokens as claim tokens.

<unknown url="https://app.notion.com/p/2468bda5828c80fe8251dc5d81c5b4aa#2428bda5828c80d38c7ffb3a9244accb" alt="external_object_instance"/>

No attack vector has been identified which uses the CALL action to call the underlying MagicLP vault contract directly from the Cauldron and before any solvency checks have been performed. However out of an abundance of caution, the MagicLP vault contract should be added as a blacklisted address.

Blacklist the MagicLP vault contract for the MagicLP cauldron CALL action.

When adding or removing an asset there can be unexpected affects to the price of the vault shares. There will be a stepwise increase in the vault shares price if someone frontons the addition transaction and sends tokens to the vault, or they exist there already. And there will be a stepwise decrease in the vault shares if an asset is removed that includes a nonzero balance for the vault, this could cause some borrowing positions to go under water immediately.

Be aware of these risks and ideally only configure assets before the system is live and do not edit them until all claims have been made.

Any accumulated yield for the existing MagicLP system will be trapped after the upgrade.

Consider claiming the current outstanding yield and understand that any yield which accumulates after the upgrade will have to be rescued.

The claim function performs no validation to ensure that the shares value provided is nonzero. As a result anyone can call the claim function even if they are not holders of MagicGLP and cause the LogClaim event to be emitted.

Consider validating that the shares value provided is nonzero to avoid any unexpected execution paths or event emissions.

Currently the MagicGLP cauldron is disabled because a reverting oracle is in place, however as soon as the MGLPV2Oracle is put in use the cauldron will become functional again. If the MagicGLP upgrade is not completed before the cauldron is re-enabled, stepwise jumps in the value of MagicGLP shares may occur and cause unintended liquidations before all assets are tracked.

Be sure that all redemption assets are sent to and are being tracked by the MagicGLP contract through the MagicGlpRewardHandlerV2 implementation before the new Oracle is assigned to the Cauldron contract.

The GLP tokens held by the Grayhat are planned to be burned by the GMX team to effectively redistribute this amount to the rest of GLP holders. This will cause a stepwise increase in the value of the MagicGLP shares as this value is appropriately re-distributed to GLP holders. If the cauldron were to become re-activated before this burn takes place then users could be unfairly liquidated, as their GLP holdings (through MagicGLP) have not been updated in value.

Consider waiting until the grayhat’s GLP shares have been burned before setting the new non-reverting oracle implementation on the Cauldron.

The MGLPV2Oracle oracle reverts with a divide by zero revert when all shares of the vault have been redeemed and there is no TVL left in MagicGLP. As a result, for the final claimer, if they happen to have a borrow position and attempt to use a flash-close method to withdraw their collateral and subsequently repay it, the solvency check at the end of the cook action will fail due to the oracle divide by zero revert.

Be aware of this quirk and consider returning zero as a result from the MGLPV2Oracle when the TVL amount is zero.