Findings:
- [CRITICAL[ infinite money printing glitch (see below)
- Fixed by fixing bug in AssociateDebt module which allowed for markets which were not in range to assign debt to an account. Tests added. https://github.com/Synthetixio/synthetix-v3/commit/eea624a739090ff5ace028de4ea25ffc1c926e96
- [CRITICAL] LegacyMarket.sol does not implement the
onERC721Received
function. As a result, callingmigrate
will always revert as minting an account token to itself will fail.- added
onERC721Received
function https://github.com/Synthetixio/synthetix-v3/commit/5fe3425b23b3c0c6e21cbab42b25f7832fcfde54
- added
- [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.- our in-house keeper will automatically migrate accounts before they go underwater is the plan. its just too much to deal with something more sophisticated than this atm
- [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 thetmpLockedDebt
always equals zero when thereportedDebt
is zero, which consequently causes the market'sreportedDebt
to augment during the migration. To maintain the market'sreportedDebt
fixed during this transition, it is advisable to implement a boolean lock instead of checking whethertmpLockedDebt == 0
. For instance, returningtmpLocked ? tmpLockedDebt : iss.debtBalanceOf(address(this), "sUSD")
- implemented suggested fix. changes were related to
onERC721Received
changes so those were mashed into a single commit https://github.com/Synthetixio/synthetix-v3/commit/5fe3425b23b3c0c6e21cbab42b25f7832fcfde54
- implemented suggested fix. changes were related to
- [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 thetransferFrom
function or a specialized external function in the Synthetix contract.- fixed through changes on the v2x side to use an alternative transfer function that is already in use by the debt migrator (whic moves debt from v2x to v3) https://github.com/Synthetixio/synthetix/commit/a60a474dbefb3e7d0e74e2359bfc0bb244fa007e
- [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.- fixed by changes to v2x system by requiring permissions to create escrow entry, as suggested https://github.com/Synthetixio/synthetix/commit/a60a474dbefb3e7d0e74e2359bfc0bb244fa007e
- [MEDIUM] The
associateDebt
function in theAssociateDebtModule
uses an incorrectdebt
value when calling_verifyCollateralRatio
. This is due toassignDebtToAccount
assuming that the account’s debt has already been consolidated. However, the proceeding call todistributeDebtToAccounts
breaks this assumption, resulting in an inflatedupdatedDebt
amount. In the context ofLegacyMarket
, 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
- fixed by implementing reccomended change https://github.com/Synthetixio/synthetix-v3/commit/eea624a739090ff5ace028de4ea25ffc1c926e96 and added test to verify that the correct amount of debt can be assigned before it dies
- [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.
- this will not be fixed because the SC/TC will be progressively cutting off SNX rewards from v2x and redirecting them to v3 counterparts, so even if SNX rewards are redistributed there will be a lot less of them, eventually none. also, sUSD rewards are burnt now so there is no need to distribute those either.
- [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 fromonlyOwner
to a different role to allow a bot account to perform account liquidations without having access to theowner
private key. Depending on the final implementation, a separatemigrateAndLiquidate
could be implemented that is permissionless to remove the dependency on privileged accounts.- due to the temporary nature of the transition period from synthetix v2x to v3, we are planning to keep migrateOnBehalf to owner/permissioned only to allow for forced migraiton. I realize it could be permissionless, but it adds a bit of extra complexity that can be generally avoided if we just write a bot manage the situation internally. for all we know, there is a chance this functionality will not end up being needed for liquidations if crypto markets rise.
- [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.- the only reason we have this function set as a setter is to allow for the contract to be "cannon friendly" which prevents issues due to re-initialization if necessary. the v3 address is more or less set in stone, so we would have much bigger problems if we end up having to change it.
- [INFO] Legacy Market currently uses
createAccount(uint128)
for migration, when it should usecreateAccount()
.- I'm torn on whether or we should actually make this change. on one hand, it reduces the possibility of griefing. but on the other hand, not using a preset/known account value breaks multicall functionality in case we want to wrap the migration in the UI later. seems like a thing to review as a design consideration.
- [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.
- definitely shouldn't be possible becuase not just anyone could liquidate the legacy market and v2x doesn't have the ability to verify the liquidity ratio of legacy market which doesn't hold any collateral but, just to be sure https://github.com/Synthetixio/synthetix-v3/commit/6d58a4961fbd5e36201d055e10942167d732ecf0
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
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.