Skip to content

Instantly share code, notes, and snippets.

@iosiro-security

iosiro-security/0_legacy-market-feedback_2023-06-29.md Secret

Last active June 30, 2023 07:31
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save iosiro-security/6d07d39a8ac0807a926771212d5ba7e7 to your computer and use it in GitHub Desktop.
Save iosiro-security/6d07d39a8ac0807a926771212d5ba7e7 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
0_legacy-market-feedback_2023-06-29.md

Findings:

  • [CRITICAL] LegacyMarket.sol does not implement the onERC721Received function. As a result, calling migrate will always revert as minting an account token to itself will fail.
  • [CRITICAL] Accounts with an unhealthy collateralization ratio cannot be migrated. During migration, the call to associateDebt will validate that the debt association does not put the account into a state that can be liquidated. As a result, an account on v2 that should be liquidated cannot be moved to v3 if the collateralization ratios are the same between the two systems.
  • [CRITICAL] In the event that collateral is delegated to the preferred Pool and subsequently, any debt is migrated when the reportedDebt is zero, the debt is dispersed among the delegators rather than solely being assigned to the migrated account. This happens as the tmpLockedDebt always equals zero when the reportedDebt is zero, which consequently causes the market's reportedDebt to augment during the migration. To maintain the market's reportedDebt fixed during this transition, it is advisable to implement a boolean lock instead of checking whether tmpLockedDebt == 0. For instance, returning tmpLocked ? tmpLockedDebt : iss.debtBalanceOf(address(this), "sUSD")
  • [CRITICAL] The migrateOnBehalf function will revert unless an account has already approved the legacy market to transfer SNX on its behalf. As a result, forced migrations (including during liquidations) will not be possible. To retain this functionality, the Legacy Market will need a privileged method for accessing users’ SNX, such as a special case in the transferFrom function or a specialized external function in the Synthetix contract.
  • [HIGH] Due to the 1,000 escrow entry limit when migrating, any further entries created beyond that hard limit will be credited immediately to an account. A malicious user aware of this change could create 1,000 1 wei escrow entries and then stake for the minimum period. If they were then to migrate, they would immediately vest rewards from that period. Rewards could be accumulated over several weeks and then claimed all at once. To prevent this, it is recommended that the createEscrowEntry be restricted to system contracts that require the functionality.
  • [MEDIUM] The associateDebt function in the AssociateDebtModule uses an incorrect debt value when calling _verifyCollateralRatio. This is due to assignDebtToAccount assuming that the account’s debt has already been consolidated. However, the proceeding call to distributeDebtToAccounts breaks this assumption, resulting in an inflated updatedDebt amount. In the context of LegacyMarket, this results in accounts needing additional collateral proportional to the collateral already delegated to the market. The more collateral already delegated, the less additional collateral needs to be transferred: collateral = collateral * (1 + collateral / (collateral + alreadyDelegated). A possible fix would be to reconsolidate that account debt, e.g. int256 updatedDebt = epochData.consolidateAccountDebt(accountId) instead of relying on the value returned byassignDebtToAccount
  • [MEDIUM] v2 Staking rewards earned by the Legacy Market cannot be claimed. Rewards that are unclaimed by the Legacy Market will be fed back into staking rewards the following week for other v2 stakers. As the Legacy Market debt position in v2 will grow as accounts migrate, there will be an increasing counterincentive to migrate to v3 as the rewards increase.
  • [LOW] The minDelegationTime needs to be set to at least one week on v3. If not, it will be possible for v2 stakers to migrate from v2 to v3 to bypass the minimum staking period enforced in v2 by creating a position in v2, migrating to v3, closing their position in v3.
  • [INFO] If the migrateOnBehalf is intended to be used for liquidations, the access control should be changed from onlyOwner to a different role to allow a bot account to perform account liquidations without having access to the owner private key. Depending on the final implementation, a separate migrateAndLiquidate could be implemented that is permissionless to remove the dependency on privileged accounts.
  • [INFO] setSystemAddresses does not remove the previous approval for the v3 system. If the address changes, the original approval will remain. As the legacy market does not hold SNX itself and as the v3 system address is unlikely to change, there’s not much risk.
  • [INFO] Legacy Market currently uses createAccount(uint128) for migration, when it should use createAccount().
  • [INFO] It should not be possible to migrate the Legacy Market itself. A liquidation bot may detect that the Legacy Market’s position is undercollateralized (as it holds debt and no collateral) and attempt to liquidate the Legacy Market position. As the liquidation logic is unknown at this point, it is unknown the extent to which this would be possible.

Infinite money bug when migrating using Legacy Market [CRITICAL]

Description

Using Legacy Market (LM) to migrate v2 accounts to v3 and transform v2 debt to v3 debt, it was possible to force the LM into a condition of unbacked debt which could be exploited to mint an unlimited amount of v2 sUSD. Fundamentally, this issue is due to LM debt only being able to increase while credit available to the LM can decrease.

The LM tracks its total debt as reportedDebt + netIssuance, with reportedDebt representing the amount of v2 debt migrated to the market and netIssuance representing the amount of v2 debt converted to v3 debt in the market. When an account from v2 migrates to v3, its collateral is delegated to the preferred pool, while its debt is directly allocated to the account. The account debt is also accumulated in the market's reportedDebt value. The convertUSD() function can be called to burn the migrated v2 debt, decreasing reportedDebt, issuing the caller with v3 snxUSD, which increases netIssuance. Using these functions it is possible to increase or transform LM debt, however, it is not possible to decrease the LM's total debt. Furthermore, an account could pay off their debt by burning snxUSD, to be able to undelegate from the preferred pool and withdraw their SNX from v3.

If an account were to repeat the following process, they would be able to reach a point where the total debt of the LM would be larger than the total collateral supporting it. 1. Account with SNX issues max synths as sUSD on v2. 2. Migrate account to v3 (this increases credit to LM, as well as its reportedDebt). 3. Convert sUSD to receive snxUSD (decreases reportedDebt but increases netIssuance by the same value). 4. Burn snxUSD and pay off the v3 account's debt. 5. Undelegate the account's collateral from the pool and withdraw their SNX (decreases credit available to LM)

Since accounts are over-collateralised this process needs to be repeated until reportedDebt + netIssuance >= creditCapacity, if there is only one account in the market then creditCapacity is equal to the account's collateral value. At this stage, the pool will be detached from the market. When the pool no longer supports LM, the debt allocated to the account is a fraction of the value it should be. To pay this debt the attacker could source a small amount of snxUSD on the open market since a call to convertUSD will fail without the pool.

Once LM is no longer supported by a pool, any account that migrates using LM will receive 0 USD of debt and would be able to undelegate and withdraw their collateral after the minimum delegation time. As such an attacker could flash-loan SNX and repeat the below process to profit:
1. Issues max synths as sUSD on v2.
2. Migrate account to v3.
3. Undelegate collateral from the pool and withdraw their SNX.
4. Repeat the above 3 steps.

This attack would mint an unlimited amount of sUSD to the attacker and upon completion of the attack could return any loaned SNX.

A POC test case for the attack is available at: https://gist.github.com/keanumaharaj/978723d8c31dd4c02698e9489b86d8e2

Recommendation

The root of the issue is a monotonically increasing netIssuance together with migrated accounts being able to pay off their individual debt with no impact on the market's debt. In the current design of v3 there is no clear solution without implementing LM specific functions within other modules.

A possible approach would be to prevent migrated accounts from being able to pay off their individual debt using burnUsd() and to instead force them to use a custom function in LM that decreases the market's totalDebt() proportionatly to the amount of debt the account is paying.

If a minimumCredit limit were to be enforced by the market to ensure the market always has sufficient capacity for the V2 debt, other migrated accounts could become locked indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment