Auto-Compounding LP Vault

RewardsDistributionRegistry.claimRewards() uses claimRewardsFromDistributor() to claim the rewards from the Synthetix core system. Once done, the tokens will be transferred to the RewardsDistributionRegistry contract. The rest of the claimRewards function will perform unwrapping (if needed) and will send the newly claimed amount to the msg.sender, which is expected to be the AutoCompoundLP.

However, the claimRewardsFromDistributor function is public which means anyone can call it, and claim the rewards.

The Synthetix core will send them again to the RewardsDistributionRegistry with no way to get them out of there.

eEven if claimRewards() is called, as already mentioned, only the newly claimed amount will be sent to the user. In result, the rewards are stuck and the vault loses its revenue. The attack can be repeated again and again for as cheap as paying gas fees.

It's expected that all users can use RewardsDistributionRegistry to claim their rewards, not just the AutoCompoundLP, therefore they are also impacted by this vulnerability.

Make claimRewardsFromDistributor() internal

Every time a user receives AutoCompoundLP tokens in result of a deposit, their userLastDeposited mapping is updated with the timestamp at which the deposit happen. This value is then used to determine two things:

• whether the tokens of the user can be transferred (they can't if a given period of time hasn't passed since the last deposit time)
• calculate the amount of penalty the user should pay if they decide to redeem their tokens (the sooner the deposit, the greater the penalty)

The AutoCompoundLP.deposit() function checks that the specified receiver is the msg.sender if the function is not invoked by the LPMigrator. This protects users from becoming a target of a small deposit with the goal of reseting their userLastDeposited mapping.

If we take a look at LPMigrator.migrate(), which is responsible for calling AutoCompoundLP.deposit(), we will see that the receiver passed is never verified - it can be any arbitrary address. This allows malicious actors to bypass the receiver restriction in the following way:

• create a brand new account with no debt and no collateral
• send a small amount of USDC to LPMigrator and a desired recipient
• The recipient receives the AutoCompoundLP tokens and their userLastDeposited mapping is updated
• They cannot transfer their tokens until the period passes and have to also pay the full penalty amount on redeem

These steps can be repeated again and again.

Don't allow passing arbitrary receiver to LPMigration.migrate()

- IAutoCompoundLP(autoCompoundVault).deposit(usdcBalance, receiver);
+ IAutoCompoundLP(autoCompoundVault).deposit(usdcBalance, msgSender);

SNX intends to allow ERC2771 forwarders to perform transfers of SLP shares. However, since it uses its own ERC2771Context._msgSender() implementation and invokes the transfer function of ERC20Upgradeable from OpenZeppelin—which relies on its inherited ContextUpgradeable—the msgSender() considered during the transfer is always msg.sender.

As a result, all transactions initiated as meta-transactions for transfer will fail, since the transfer will be attempted from msg.sender, i.e., the forwarder contract itself.

function transfer(
     address to,
     uint256 value
) public override returns (bool)
     block.timestamp - userLastDeposited[ERC2771Context._msgSender()] <
            decayingRedemptionPenaltyDuration
        ) {
            revert TransferBlocked();
        }
        return super.transfer(to, value); // @audit doesnt onsider context, would attempt transfer from forwarder
 }

Consider overriding _msgSender() of context upgradable

RewardsDistributionRegistry has a function claimRewards(), as well as getRewards() function that returns the amount of rewards that can be claimed.

Both claimRewards() and getRewards() have a totalClaimed variable showing how much rewards of the given token have been claimed. It comes from claimRewardsFromDistributor() or claimRewardsFromDistributor(). The amount returned and therefore totalClaimed is always gonna be in 18 decimals precision, as we can see in the following vault logic here and here.

The problem is that if rewardIsSynth == false, rewardAmount = totalClaimed will be executed, but all of the code afterwards assumes that rewardAmount is in the decimals of the rewardToken.

For example, claimRewards() will revert for USDC because it tries to transfer rewardAmount of USDC which is in 18 decimals, resulting in DOS of the whole claimRewards() function.

getUsdValueOfRewards() makes a conversion by multiplying by 1e18 and dividing by 10^(rewardToken.decimals). For USDC, this results in a value with 30 decimals. This will report extremely large value to AutoCompoundLP and will completely break it.

Convert the rewardAmount to have the same decimal precision as the reward token on both lines:

RewardsDistributionRegistry.sol:L261 RewardsDistributionRegistry.sol:L165

The current script swaps SNX as well, which causes a spike in totalAssetsStata, potentially enabling a sandwich attack.

The SNX team does not intend to swap SNX received as rewards and instead hardcodes its price as 0 in the calculation of totalAssetsStata.

However, the keeper swaps all tokens returned from getRewards, including SNX—even when min and max are not defined for the token. The keeper simply ignores tokens without defined min and max, but still proceeds to swap them.

A further issue with this approach is that if any unexpected token is received as a reward (i.e., not currently configured), the keeper will attempt a full swap of the entire amount, potentially causing significant slippage.

Additionally, this logic can lead to an unnecessary swap attempt of stataUSDC, which should not be swapped.

Only swap tokens that are explicitly listed in the config.ts file and have both min and max defined.

Log any ignored tokens.

If the ignored token is something other than SNX, trigger an alert so the SNX team can review and update the config accordingly.

A penalty in the AutoCompoundLP contract allows an attacker to manipulate the ratio of total assets to total supply, enabling an inflation attack.

By performing a strategic deposit followed by a near-complete redemption (leaving only one share), the attacker can distort the asset-to-share ratio, causing subsequent depositors to receive zero shares for their deposits.

The attacker deposits a large amount and redeems all but one share, triggering a penalty and setting the total supply to 1 while maintaining total assets equal to the penalty.

This creates a state where a second depositor’s assets are absorbed without receiving any shares, as their deposit amount is insufficient to overcome the manipulated ratio and penalty.

The attacker can further dilute the ratio and redeem the assets after a decay window, potentially stealing the second depositor’s funds.

Consider minting dead shares equivalent to min delegation amount inside initialise.

LPMigration withdraws snxUSD from the user's position in _withdrawAvailableSnxUsd() and synth in _undelegateAndUnwrapSynth(). The amount to be withdrawn is fetched from getAccountAvailableCollateral(). However, this doesn't account for any active collateral locks.

For example, user has 500 available collateral, but 100 of them is locked, which means at most 400 can be withdrawn. The migrator will try withdrawing all of it (500) and the transaction will revert, resulting in inability for the user to migrate using the contract until their lock expires.

Take into consideration the locked amount when computing how much collateral is to be withdrawn.

The POST /lp-vault-keeper endpoint is exposed without any authorizer, API-key, or IAM requirement, enabling anyone on the Internet to invoke sensitive Lambda logic.

Even though the Lambda runs in private subnets, the API Gateway lives outside the VPC; it exposes a public HTTPS URL and then calls the Lambda over AWS’s internal channel. The VPC blocks direct inbound traffic to the function, but it does nothing to hide the Gateway itself, so anyone who knows the URL can still invoke the endpoint

Require authentication — add a JWT/Lambda authorizer, IAM auth, or at minimum an API-key with usage-plan limits.

The SNX team assumes that checking if (balanceAfter - balanceBefore) < assetsReceived is sufficient to prevent misuse in case of keeper compromise.

They specifically mention this safeguard to prevent a scenario where the keeper sets themselves as the receiver:

"The main way that we saw this being possible is swapping tokens out to a separate to address. This is accounted for by the smart contract checking the returned value of the odos swap—which is the USDC received—and asserting that it equals the difference in USDC balance before and after the swap."

Notion Link

However, this assumption is flawed. assetsReceived does not validate the output token itself.

A malicious keeper could perform a swap into a token with a lower value per unit than USDC, misleading the contract into believing USDC was received.

If the goal is to validate assetsReceived, ensure the output token is explicitly checked.

That said, even with such validations, a malicious keeper could still find other ways to extract funds, so further defense-in-depth mechanisms should be considered.

For example: M-06

Whenever the keeper receives USDC as rewards, it will revert.

This happens because the keeper attempts to swap USDC to USDC, and the Odos API reverts when the input token is the same as the output token.

Consider skipping USDC during inputTokens filtering.

The keeper script, and therefore the vault, will execute whatever swap is quoted by the ODOS API.

Since ODOS is an external dependency, blindly trusting its output is risky.

Additionally, the keeper uses an EOA.

If the private key leaks from any developer machine, the contract functions would be exposed.

Add validations for swap quotes returned by ODOS. These checks must execute within 60 seconds to ensure the quote does not expire.

The validation could involve simulating the claimSwapCompound function and checking the delta in totalAssetsStata. If the delta exceeds a certain threshold, the swap should be rejected.

For improved key management and security, consider adding this validation for delta between before and after keeper actions at the smart contract level.

Since SNX already tracks totalAssetsStata and does not expect large deviations from keeper actions, this validation can be incorporated easily.

Implementing the validation at the contract level would also safeguard SNX in case the keeper script or its private key is ever compromised.

SECRET_ARN and an Alchemy RPC key are committed in plaintext. Attackers with repo access can extract production secrets or drain paid API quotas.

Store secrets exclusively in AWS Secrets Manager / SSM Parameter Store and reference them via environment variables blocked by pre-commit secret-scans.

getPrivateKey() returns the raw string and downstream code could log or crash with key in memory.

Return an AWSKMS-wrapped Signer (e.g., ethers.Wallet.fromEncryptedJson) and zero sensitive buffers after use.

API Gateway lacks usage plans and per-method throttling; attackers can exhaust Lambda concurrency and upstream quotas.

Apply a usage plan (e.g., 100 req/min) and set Reserved Concurrency on the Lambda.

allowAllOutbound: true permits the lambda function to connect anywhere, creating an exfiltration path if compromised and malicious code was injected into the lambda.

Restrict egress to 443 toward approved FQDNs/CIDRs (Alchemy, Odos, AWS APIs) via prefix-lists or explicit rules.

The public API is not fronted by AWS WAF, exposing it to OWASP Top-10 attacks and volumetric abuse.

Attach AWS WAFv2 web-ACL to the API stage; enable managed rules (AWSManagedRulesCommonRuleSet, BotControl).

In LPMigration._clearDebt(), the usdcAmount needed to repay the debt is increased by 1 to account for rounding errors. Notice that this is not rounding up, it happens even if the result is a round number.

Because of this, a user may end up in a situation where he has X amount of debt and X amount of USDC in their wallet, but the code will calculate the usdcAmount as X + 1.

Because of this, the X will be repaid by the user's wallet and the 1 will be taken as a flashloan.

However, the user's X is enough to repay the whole debt and as result it will be erased.

This will make debtAmount - repayableDebt = 0, and executeOperation() will try selling 0 sUSD. Because the sUSD.burn() function reverts if the amount to be burned is 0, the whole transaction will be unsuccessful.

You can implement one of the following:

1. Round the usdcAmount up only if needed
2. Skip the flashloan if the new debt is 0

                debt = ICoreProxy(CoreProxy).getPositionDebt(
                    accountId,
                    poolId,
                    collateral
                );

                if (debt == 0) {
                    return;
                }

                _aavePool.flashLoanSimple(
                    address(this),
                    $USDC,
                    usdcAmount - userUsdcBalance,
                    abi.encode(
                        accountId,
                        debtAmount - repayableDebt,
                        debtAmount - repayableDebt,
                        collateral
                    ),
                    0
                );

LPMigration uses CollateralModule.deposit() when clearing the positive user debt. This action processes all locks of the user (999999999999999 is used as count for cleanAccountLocks). Because of this, accounts with too many locks may require too much gas to be migrated, potentially even blocking the migration.

Inform users about locks increasing the cost of the migration.

AutoCompoundLP.totalAssetsStata() adds the value of the reward tokens held by that contract towards all the available assets. ShouldstataUSDC be added as a reward tokens, _convertToAssetsStata calculations will return wrong results. For example, when users deposit, their USDC is turned into stataUSDC and then _convertToAssetsStata() is called. Having stataUSDC itself as a reward token will lead to inflation in the amount of available assets and thus the user receiving wrong amount.

Don't add stataUSDC as reward token for as long as the code performs the assets calculation like it does now.

AutoCompoundLP.totalAssetsStata() adjusts the available assets amount by converting the absolute value of netNonNativeAmount to stataUSDC by dividing by the rate().

In cases when netNonNativeAmount is negative, the mulDiv will round the result up. For example, if the debt is -5.5, the function will round it as 5, not 6 and in result totalAssetsStata will be inflated.

Consider rounding in the opposite direction if netNonNativeAmount is negative. This will cause precision loss in the other direction, but it's generally better to overestimate loses compared to underestimating them.

WrapperModule.wrap() and WrapperModule.unwrap() take an amount of tokens to wrap/unwrap, applies a fee to it and returns the result in the appropriate token. For the scope of this audit, the fees are assumed to always be 0. However, let's look at the way the amounts are computed.

uint256 usdAmount = baseAmountD18.mulDecimal(synthPrice);
synthAmount = usdAmount.divDecimal(synthPrice);

These 2 lines are taken from wrap, but they are the same in unwrap (the names are different). Let's replace mulDecimal and divDecimal. Then we got

uint256 usdAmount = baseAmountD18 * synthPrice / 1e18;
synthAmount = usdAmount * 1e18 / synthPrice;

Because of the first division, usdAmount can experience a loss up to (but not including) 1. This loss is then amplified by the multiplication with 1e18 / synthPrice, resulting in a maximum possible loss of up to floor(1e18 / synthPrice + 1) (+1 because of the division on the second line). For synthPrice > 1e18 the maximum loss should be capped to 1, but the lower the price is, the bigger the potential precision loss experienced.

To handle that, RewardsDistributionRegistry._unwrappedAmount() subtracts 1 from the expected received amount. As mentioned above, this should be fine for synthPrice > 1e18, but can result in reverting transactions if the price drops below that.

Now let's look at the wrap() that happens in _increasePosition. The synth market used is the synthIdStataUSDC. The synthAmount expected to be received doesn't account for the precision loss. At the moment of writing, indexPrice of that market returns a value which has its last 12 digits as 0s. If this property stays true, then there is no need to account for the precision loss because the converted synthIdStataUSDC will also has its last 12 digits as 0s which means there is no precision loss at all. However, if the price were to change its format, then the code should account for that by adjusting the slippage tolerance as well.

There are 2 more types of wrap/unwrapped() that haven't been discussed:

• unwrap synthIdStataUSDC - there is no need to account for the precision loss here because after it happens, the amount is divided by 1e12. This means the part where the precision loss happened is irrelevant. Keep in mind that if the decimals of the stataUSDC change (highly unlikely), you may need to adjust the expected returned amount as well.
• wrap/unwrap synthIdUSDC - the price of this market is hardcoded to 1e18, which means that when the 2 lines of code in this report are executed, there won't be a precision loss.

Consider if any of the behaviors mentioned in the report should be taken into account and if so, change the slippage protections accordingly.

When removeDistributor() is called, it will remove the distributor from distributorData and distributors. This will make any rewards that have accrued from that distributor and not been claimed yet to be lost. This happens because distributors loop through distributors, and will no longer be able to claim them.

Prior to deleting the data for a distributor, claim the rewards from that distributor first from AutoCompoundLP.sol.

The AutoCompoundLP contract's core functions, deposit and compound, rely on the internal _increasePosition function. This function wraps stataUSDC into synthStataUSDC by calling IWrapperModule.wrap on the Synthetix Spot Market (spotMarketProxy).

The Synthetix wrap function enforces a market-wide maxWrappableAmount limit for each specific synth market. For synthStataUSDC (market ID 3), this limit is currently set at 50 million units, represented as 50000000000000000000000000 (5e25) when scaled to 18 decimals, according to Synthetix V3 configuration (e.g., found in meta.json within node modules).

If this market-wide limit (maxWrappableAmount) is reached due to aggregate wrapping activity across all protocols and users interacting with the Synthetix synthStataUSDC market, subsequent calls to wrap for market ID 3 will revert.

This reversion will directly cause failures in:

1. deposit transactions initiated by users attempting to add liquidity to the AutoCompoundLP vault.
2. compound transactions initiated by keepers attempting to reinvest rewards. This, in turn, can cause redeem() to revert. This will occur because the calculation for assets takes the rewards into consideration. Therefore, a user may get an overestimated amount of USDC as a reward, that can not be sent to them due to compound() reverting.

This leads to a Denial of Service (DoS) for these essential vault operations. The AutoCompoundLP vault's ability to accept new deposits and to auto-compound rewards becomes entirely dependent on the available capacity within the external Synthetix protocol's wrapper limits for synthStataUSDC.

If this limit proves to be a frequent bottleneck for the AutoCompoundLP vault's operation, the recommended long-term solution is to engage with Synthetix governance to request an increase in the maxWrappableAmount for the synthStataUSDC market (ID 3).

Furthermore prepare communication strategies for users and keepers should the limit be approached or reached, explaining why deposit or compound operations might be temporarily unavailable due to the external Synthetix limit.

This AutoCompoundLP.setRewardsRegistry is an owner-only function allows changing the address of the rewardsRegistry contract. The AutoCompoundLP vault uses this registry to claim and value Synthetix rewards associated with its Synthetix accountId. The function performs two main actions:

1. Updates the rewardsRegistry state variable to the newRegistry address.
2. Grants the necessary REWARDS_PERMISSION within the Synthetix system to the newRegistry address for the vault's accountId.

The setRewardsRegistry function lacks a mechanism to ensure that any pending rewards, claimable only through the old rewardsRegistry address, are claimed before the switch to the newRegistry occurs. These pending rewards will become permanently inaccessible (if all the old reward distributors are not registered in the newRegistry) via the vault's standard functions listed below:

• claimRewards
• claimSwapCompound
• totalAssetsStata (which internally calls getUsdValueOfRewards)

This scenario leads to a potential permanent loss of funds for the vault and its depositors, as the unclaimed rewards become stranded.

To prevent the potential loss of accrued rewards during a registry update, it is recommended to claim the pending rewards of the old rewardsRegistry, before the owner calls setRewardsRegistry.

AutoCompoundLP.sol::claimSwapCompound calls RewardsDistributionRegistry.sol::claimRewards to process rewards.

For synthetic rewards (rewardIsSynth=true), claimRewards loops through distributors and calls _unwrapSynth. This function uses _unwrappedAmount to determine the minimum underlying tokens expected (minAmountReceived) for the IWrapperModule.unwrap call.

_unwrappedAmount function calculates this by subtracting exactly 1 wei from the expected amount, aiming to cover rounding errors. Crucially, this calculation does not factor in the unwrapFixedFee specific to the Synthetix spot market (spotMarketId) being used for that distributor.

If any distributor's market has an unwrapFixedFee configured, the unwrap call will receive expectedAmount - fee, which is less than the required minAmountReceived (expectedAmount - 1 wei). This triggers a revert within IWrapperModule.unwrap due to failed slippage protection.

Because this check occurs inside the claimRewards loop, the failure of even a single distributor's unwrap reverts the entire claimRewards function call. Consequently, the calling function claimSwapCompound also reverts. This leads to a Denial of Service (DoS) vulnerability, preventing keepers from successfully claiming and compounding any rewards system-wide until the problematic distributor is removed or its associated market fee configuration changes.

Modify the _unwrapSynth function (or its helper _unwrappedAmount) within RewardsDistributionRegistry.sol to correctly calculate the minAmountReceived passed to IWrapperModule.unwrap, ensuring it accounts for potential fixed unwrap fees. This can be done by querying the minAmountRecieved amount by calling the quoteUnwrap function of the respective spotMarket.

As per known issues for the vault, SNX is aware that performSwaps approves tokens to the ODOS router even if they are not intended to be swapped—specifically SNX, stataUSDC, and USDC.

SNX’s position is that the keeper would never attempt to swap these tokens.

However, DEX routers have previously been compromised to exploit unused allowances.

So even if the keeper does not initiate swaps for these tokens, they could still be maliciously swapped with different receivers if the router is compromised.

Only approve tokens that are actually required for the intended swap.

SNX allows the owner to update the rewards registry if needed.

However, when this change occurs, the REWARDS_PERMISSION for the old rewards registry is not revoked.

Consider revoking REWARDS_PERMISSION from the old rewards registry upon updating to a new one.

SNX uses stataUSDC as collateral for this vault’s account ID.

Each user’s USDC deposit is sent to AAVE, which returns stataUSDC. This token is then deposited and delegated to SNX.

During redemption, stataUSDC is retrieved from SNX Core and then redeemed back to USDC.

Debt repayment also includes the redemption of stataUSDC to USDC.

However, AAVE enforces both maxDeposit and maxRedeem limits:

Contract Link

These limits could potentially cause reverts during user interactions.

While current AAVE limits are unlikely to affect SNX operations, it's important to monitor them.

As TVL grows or if AAVE modifies its caps, vault actions like deposit, redeem, or debt repayment could begin reverting.

Be aware of this possibility and consider implementing safety checks or fallback mechanisms.

SNX does not anticipate trading activity involving trading shares, so this is not currently a major issue.

However, both the SNX team and any integrators should be aware that there are different representations of totalAssets.

The cached value may not reflect the current state of the vault.

For any external actions involving the vault, depending on the share value, either sync functions should be used or refreshTotalAssetsCache should be called.

Integrations should be especially cautious when choosing which method to use, and should not assume full ERC4626 equivalence, as this vault implementation is not fully compliant.

N/A – This is added as a reference for integrators to consider during implementation.

The RewardsDistributionRegistry has the USDC value hardcoded as 1e18.

This can lead to inaccurate share valuations if USDC depegs, as it did briefly in the past.

While SNX’s OracleManager nodes also configure USDC as a CONSTANT node (valued at 1e18), this can be updated to use a real Pyth price feed if needed.

However, this flexibility does not exist in the RewardsDistributionRegistry, where the value is fixed.

Consider fetching the USDC price from coreProxy as well, to ensure dynamic and accurate pricing.

The current approach in RewardsDistributionRegistry is to add each synth reward underlying token in the rewardTokens array and map it to the synth in the rewardTokenToSynthToken mapping.

This is done only if the reward token is not already added to the array. Even if the distributor is removed, these data structures are not changes. There are several security consideration around this behavior:

• the call to getCollateralPrice() is not wrapped in a try/catch, which means if it reverts for an old token, usdValueOfCallerBalance() will be DOS-ed
• If the same reward token is added with a different synth, the rewardTokenToSynthToken mapping won't be updated and it will keep pointing to the old synth
• If the array becomes too long, looping over it may cause OOG revert

Reconsider the approach to never delete these values on distributor removal

The decayingRedemptionPenalty function calculates the penalty amount to be deducted during a redemption. The final calculation assets.mulDiv(timeAdjustedFeePercent, PERCENT_PRECISION) uses the default behavior of mulDiv, which rounds the result (the penalty amount) down.

Rounding the penalty amount down slightly benefits the withdrawing user, as they pay a smaller penalty than if rounding were neutral or upwards. This contradicts the rounding approach potentially used elsewhere, such as when converting shares to assets (e.g., _convertToAssetsStata uses Math.Rounding.Floor), which explicitly rounds the user's entitled assets down, favoring the protocol.

To ensure rounding consistently favors the protocol during redemptions involving penalties, modify the final calculation within decayingRedemptionPenalty to use ceiling division that rounds the penalty amount up.

The protocol uses transfer() and approve(), instead of safeTransfer() and safeApprove(). Some ERC-20 tokens have custom logic, such as USDT on ethereum mainnet’s approvals must be reset to zero before creating a new approval. This can lead to unexpected reverts throughout the protocol.

Switch all instances of transfer() and approve() to safeTransfer() and safeApprove().

The return value of the rewards function is not validated, which could result in an empty rewards list being passed to inputTokens.

Consider adding validation to check the return value of the rewards function.

The getRewards function only returns newly claimable rewards, and does not account for tokens already held in the vault's balance.

For example, if token A had rewards in the last epoch and was partially swapped—leaving the rest in the vault for periodic swapping—then in the next epoch, if claimableRewards for token A is zero, it won’t be included in getRewards. As a result, the keeper won’t attempt to swap it.

Consider iterating through the internal rewards array in the keeper script and logging the vault's balance of each token during populateTokenAmounts.

The current getOdosQuote error handling is incomplete for responses with status codes 422 and 502, as these do not return an errorCode in the expected schema.

Consider checking for a successful response explicitly, rather than relying on the absence of failure. Revert if a successful return is not detected

The assembleOdosQuote function lacks error handling and does not check for deprecation status.

This could lead to incorrect or expired calldata being used in execution.

Consider adding proper error handling in assembleOdosQuote, including a check to ensure the quote is not marked as deprecated (the first field in the response).

There is no tracking for the keeper's ETH balance or the gas used.

This could result in keeper transactions failing due to insufficient funds for gas.

Consider adding logs for gas used and remaining ETH balance, along with an alert system to notify admins if the balance falls below a defined threshold.

ODOS requires all addresses to be checksummed according to their API documentation:

Currently, SNX does not have the following addresses checksummed in config.ts, which could result in reverts:

• 0x833589fcd6edb6e08f4c7c32d4f71b54bda02913
• 0xc1cba3fcea344f92d9239c08c0568f6f2f0ee452

Review all addresses used in the keeper infrastructure and ensure they are properly checksummed where required.

SNX currently uses Alchemy as their RPC provider.

This setup could become vulnerable to sandwich attacks if Base implements a public mempool with bribes, replacing the current FCFS (First-Come, First-Served) sequencer model.

Once Base introduces a public mempool with bribe support, consider switching to a frontrunning-resistant RPC.

Since SNX does not use a slippage configuration in getOdosQuote, ODOS applies its default slippage of 0.3%.

This may be either too high or too low depending on the specific reward token, potentially leading to suboptimal or failed swaps.

Consider adding a slippage parameter in config.ts for each reward token and using it when requesting quotes from ODOS.

The migrate function in LPMigration.sol is designed to unwind a user's Synthetix V3 position (collateral and debt) and deposit the net USDC value into the AutoCompoundLP vault. However, the current implementation only handles the principal collateral ($synthUSDC, $synthStataUSDC) and associated debt (snxUSD). It completely omits the process of claiming any pending Synthetix rewards accrued by the user's accountId for providing liquidity or collateral.

Consequently, the value of these unclaimed rewards is not included in the initial deposit made to the AutoCompoundLP vault during the migration. The user must manually claim these rewards later and deposit them separately.

This creates two primary issues:

1. Incomplete Migration & User Burden: The migration is not comprehensive, requiring extra steps from the user.
2. Missed Compounded Yield: The core problem is the delay in depositing the reward value into the AutoCompoundLP vault. During this delay, the reward value is not participating in the vault's yield generation strategy (Aave yield + vault's own compounded Synthetix rewards). When the user eventually deposits the reward value, the vault's share price will likely have appreciated due to accrued yield, resulting in the user receiving fewer shares for their rewards than if deposited initially. This represents a loss of potential compounded yield within the target vault for the user.

Enhance the migrate function in LPMigration.sol to include the claiming and processing of Synthetix rewards of the migrated position, before depositing into the AutoCompoundLP vault.

AutoCompoundLP.totalAssetsStata() applies rewardsUsdValueDiscount to the rewards available to account for fees and slippage when swapping these tokens. Any deposits and redeems executed before the swap of the reward will use the new ratio which includes the discount. Any leftover amount of the discount which was not used for fees will be distributed across the vault participants at the time of the swap, resulting in a profit for the new depositors.

There is no need for a fix because any amount not used for fees can be considered as a bonus, so documenting the issue and keeping it in mind is enough.

RewardsDistributorRegistry.registryDistributor() doesn't allow adding a distributor if the token it's responsible for doesn't have a valid price, i.e getCollateralPrice() reverts or returns 0.

However, the _getAssetPrice() function returns the result of coreProxy.getCollateralPrice(assetAddress) as price of the requested asset without doing any checks. If an error occurs at a later point in time inside getCollateralPrice, the returned value will be 0 and the given reward token will be treated as 0.

This can lead to unexpected behaviors since users can still interact with the AutoCompoundLP, but these rewards tokens will be ignored causing spikes in the value of the total assets.

Create a plan for what to do in such emergency cases and implement any changes if needed. Keep in mind that reverting is risky because once a token is added in rewardTokens , it stays there forever.

There is 1 added to usdcAmount in AutoCompoundLP._repay() to handle rounding issues.

However, the 1 is added unconditionally which creates problems for both redeem() and compound() because there the amount of USDC needed to repay the debt is computed with mulDiv and Math.Rounding.Ceil which round the number up only if the mathematical result of the division is not a whole number.

For example, consider the case where there is 1e18 debt of the AutoCompoundLP position and a user tries to redeem. The code will take a flash loan of 1e6 USDC in redeem(), but will try to wrap 1e6 + 1 because of this +1 happening, resulting in reverting redeem transaction. The same is true for the compound() function.

Replace the rounding with mulDiv.

After that, you can delete the following -1

When users redeem their AutoCompoundLP shares, _convertToAssetsStata() will update the totalAssetsCache to currentTotalAssets - assets. Here assets is the requested amount to be redeemed, but it doesn't include the penalty amount which technically doesn't leave the system and should not be subtracted. This results in a discrepancy between the real USDC value and what's cached and in turn leads to wrong results returned from previewDeposit and previewRedeem().

You can make the refreshTotalAssetsCache() visibility public and use it throughout the codebase to refresh the cache.

The AutoCompoundLP vault is a participant in the v3 core system which means it's a subject to liquidations if its collateral ratio becomes unhealthy. If that happens, both its debt and collateral will be erased, but the vault depositors will continue holding all their previous tokens. If the vault remains active after the liquidation, the old depositors will have exposure to all the funds deposited afterwards, leading to loss for the new depositors and profit for the old ones.

Consider halting the vault in case its experiences a liqudiation.

When calculating the worth of the AutoCompoundLP shares, the reward tokens held in the contract are also included in totalAssetsStata. This allows a vault participant to donate a large amount of rewards to the vault, change the share price and then redeem their shares for a large amount of the current delegated assets, leaving the vault with only minDelegationAmount or even 0 assets.

Doing this is expensive since the more assets they are in the vault, the more rewards the user has to donate, but it can lead to some unexpected behaviors, like the AutoCompoundLP not receiving rewards and liquidation rewards or depositors not being able to redeem because there is not enough liquidity.

Document this behavior.

When users redeem, if there is debt accrued by the AutoCompoundLP position, it will be repaid with an Aave flashloan.

Then the amount undelegated from the position is the amount to be received from the user + the amount to repay the flashloan.

This can cause the amount delegated to the market to fall below its minimumCredit and in result the redeem will be unsuccessfull.

Document the risk.

RewardsDistributionRegistry.claimRewardsFromDistributor() and RewardsDistributionRegistry.getRewards() call claimPoolRewards() and getAvailablePoolRewards() twice - the first time for the lpCollateralAsset vault and the second time for the address(0) vault. Usually, it's not expected to have active stakers in the vault for address(0) collateral, which makes the second call unnecessary.

Consider whether the second call is necessary and remove it otherwise.

The API key for your Alchemy account is exposed in your test cases. This can allow anyone to use your API and make calls from your account.

Use a .env file to store the API key. Then concatenate a string with the base of the URL with the API key.

Functions like getRandomVariance() and submitTransaction() are stubs; TODOs indicate unfinished logic that can mask security gaps.

Remove or complete TODOs; run static analysis to ensure no unreachable code remains.

Token lists, chain IDs, and account-specific constants are fixed in code, causing config drift across environments.

Externalise to SSM parameters/CDK context and document overrides for staging vs. prod.

memorySize:256 MB with timeout:30 s could be over- or under-provisioned, affecting cost and latency.

Benchmark under load; right-size memory (which also boosts CPU) and reduce timeout to ≤10 s if feasible.

Subnet IDs/security group IDs are imported via CloudFormation outputs without tags or documentation, hindering troubleshooting.

Tag imported resources (cdk.Tags.of(...)) and maintain design-docs/diagrams that map stack dependencies.

dataTraceEnabled is true (non-prod), writing full request/response bodies—including tokens & calldata—to CloudWatch.

Disable full traces or mask fields with log-filtering. Encrypt logs and limit reader IAM roles.

Logs are retained for ONE_WEEK — insufficient for many compliance or forensic requirements.

Extend retention to ≥90 days or export to S3 + Glacier automatically.

SecretsManager.getSecretValue() is executed on every invocation, adding latency and cost.

Cache the secret in /tmp (persist across warm invocations) or use Secrets Manager extension.

RemovalPolicy.DESTROY deletes log groups when the stack is torn down, erasing evidence needed for investigations.

Set RemovalPolicy.RETAIN in production and automate log archival to S3 before stack deletion.

fetch() to Odos/Alchemy has no timeout or retry; slow responses lead to Lambda timeouts.

Wrap calls with AbortController (≤5 s) and implement exponential back-off with jitter.

Proxy integration forwards raw payloads; the Lambda never validates schema or data types, enabling injection or resource-exhaustion attacks.

Add strong JSON schema validation (e.g., ajv) and early-return 400 on malformed requests.

if(!event.body) return {statusCode:400,body:'Missing body'};

No explicit IAM role is attached; CDK’s default role may include permissions the function doesn’t need or omit required ones.

Define a custom least-privilege role

const lambdaRole = new iam.Role(this,'Role',{assumedBy:new iam.ServicePrincipal('lambda.amazonaws.com')});
lambdaRole.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaVPCAccessExecutionRole'));
lambdaRole.addToPolicy(new iam.PolicyStatement({actions:['secretsmanager:GetSecretValue'],resources:['*']}));

The redeem function calculates the amount of USDC needed for an Aave flash loan (usdcNeededToRepay) to repay the user's Synthetix debt (debt). This calculation is performed by the usdcToRepayDebt helper function.

Currently, usdcToRepayDebt performs a direct 1:1 decimal conversion from the 18-decimal debt (denominated in snxUSD) to 6-decimal USDC. However, the actual repayment happens within the Aave flash loan callback (executeOperation), specifically inside the _repay internal function. This function determines the precise amount of synthUSDC needed to acquire the target debtAmount of snxUSD by querying the Synthetix market via IAtomicOrderModule.quoteSellExactOut.

Crucially, the quote returned by quoteSellExactOut includes potential positive skewFees imposed by the Synthetix market if the trade would increase market imbalance.

If a positive skewFee is applied by Synthetix during the quoteSellExactOut call, the required synthAmount (and consequently the usdcAmount needed in _repay) will be slightly higher than the simple 1:1 conversion amount calculated initially by usdcToRepayDebt when determining the flash loan size (usdcNeededToRepay). As a result the contract will lack sufficient USDC received from the flash loan to perform the necessary wrap operation inside _repay.

This results in the entire redeem transaction reverting due to insufficient funds during the flash loan callback execution.

To guarantee the flash loan always covers the required repayment amount, including any potential Synthetix skewFees, modify the redeem function to calculate usdcNeededToRepay based on a real-time quote from the Synthetix market before initiating the flash loan. Else recommended to apply enough buffer on the usdcNeededToRepay value before calling the flashloan such that the _repay will not revert due to insufficient funds.

In the lp-vault-keeper.ts file, the script relies on aws-cdk-lib.

However, the version used (as per package.json) is 2.186.0, which is affected by the 2L/1M vulnerability.

Additionally, the lp-vault-keeper package lists aws-sdk as a dependency. This SDK has been in maintenance mode since September 2024 and will no longer receive full support beyond September 2025.

This introduces long-term maintenance and potential security risks, especially for components relying on AWS services like Cognito, IAM, or Lambda integrations.

• Upgrade aws-cdk-lib to the latest secure version (e.g., 2.191.0 as of the time of this review).
• Migrate from aws-sdk (v2) to the modular v3 (@aws-sdk/*) packages, as recommended by AWS.

This ensures continued compatibility, improved performance, and access to ongoing security patches.

Rewards

Rewards Registry Decimal Precision

The current version of the rewards registry is not expected to handle reward tokens with fewer than 18 decimals. All configured synth rewards are standardized to 18 decimals.

Pricing, Oracles & Precision

Conversion Ratios

Wrap/unwrap operations for USDC, snxUSD, and sUSD are assumed to follow a 1:1 value ratio with no fee.

However, minor rounding deltas are known and are currently handled via manual +1/-1 adjustments in logic.

Oracle Pricing and Staleness Risk

The price of synths is derived from their underlying collateral via Pyth oracles.

It is assumed prices will generally remain fresh, as SNX uses a keeper that updates all markets hourly and key assets (e.g., ETH, BTC) at price thresholds.

However, temporary staleness remains a valid risk.

sStataUSDC Precision Assumption

It is assumed that the price of sStataUSDC will consistently end in 12 zeros (fixed-point format).

This has been validated through testing but is not guaranteed. Deviation could result in rounding or precision loss during conversions.

Keeper Behavior & Management

Trusted Keeper Execution

It is assumed that the keeper will consistently and honestly perform its upkeep duties.

Secure Keeper Key Management

The keeper operates as an EOA. It is assumed that key access is tightly controlled and will not be misused by insiders or compromised from developer machines.

Low Risk of Insider Sandwiching Attacks

It is assumed that knowledge of pending compound events will not lead to effective sandwich attacks.

This is mitigated by randomized execution timing and Odos routing.

Randomized Keeper Timing Prevents Predictability

Random variance in keeper execution is assumed to reduce predictability and MEV risks during compound swaps.

Swap Size Limits Prevent Exploitable Volume

Maximum swap sizes ensure that large swaps are broken into smaller chunks, reducing the risk of slippage or sandwiching attacks.

Separation of Claim, Swap, and Compound Logic

These actions are modular in the keeper, allowing finer control and flexibility to avoid swapping dust tokens or executing unneeded compound steps.

Vault Entry/Exit & User Behavior

Withdrawal Penalty Discourages Quick-In/Quick-Out

A decaying withdrawal penalty (e.g., 1% over 1 day) is assumed to discourage users from quickly entering and exiting the vault in response to large trader liquidations.

24-Hour Transfer Lock Prevents Penalty Circumvention

Vault shares are non-transferable for 24 hours after deposit.

This is assumed to prevent circumvention of withdrawal penalties via share transfer to another address.

Timeout Overrides During Migration

• For the LP vault, _CONFIG_SET_ACCOUNT_OVERRIDE_WITHDRAW_TIMEOUT will be used.
• For migration contracts, _CONFIG_SET_SENDER_OVERRIDE_WITHDRAW_TIMEOUT is planned (not yet deployed on Base V3).

These allow per-user or per-sender timeout bypassing when needed.

TotalAssetsSync Includes Uncompounded Rewards

Uncompounded rewards are reflected in totalAssetsSync.

It is assumed this may introduce temporary valuation mismatch during mass redemptions, but is acceptable due to flexibility in triggering compounding.

Partial ERC4626 Compliance

Deviation from ERC4626 Interface

The vault does not fully implement the ERC4626 standard.

Functions such as totalAssets may require state-changing operations and are not strict view functions.

It is assumed that integrators are aware of this and will use the provided alternatives (e.g., totalAssetsSync) instead of expecting strict interface conformance.

Beware of these trust assumptions, a change in any of these may require a code change.

The claimRewardsFromDistributor function in RewardsDistributionRegistry.sol is currently public. While an access control check (if (msg.sender != coreProxy.getAccountOwner(accountId)) revert UserNotAccountOwner();) was added to ensure only the account owner can initiate the claim from the Synthetix coreProxy, severe issue remains.

If an account owner calls claimRewardsFromDistributor directly, the rewards are claimed from the coreProxy and are held by the RewardsDistributionRegistry contract. However, the primary claimRewards function is designed to process and transfer rewards that are claimed during its own transaction execution.

Specifically, claimRewards iterates through distributors, calls claimRewardsFromDistributor for each, and then transfers the totalClaimed amount (which is the result of this specific internal call) to msg.sender. If claimRewardsFromDistributor was called directly in a previous, separate transaction, the rewards from that initial call are now sitting in the RewardsDistributionRegistry. When claimRewards is subsequently called, getRewardsFromDistributor will likely return 0 for that distributor.

As a result, the rewards claimed through a direct, prior call to the public claimRewardsFromDistributor function are never transferred to the user by the claimRewards function and become permanently stuck in the RewardsDistributionRegistry contract.

Move the check back to the old function (for gas savings) and change the visibility of claimRewardsFromDistributor to internal.

SNX introduced a /tmp check so the Lambda looks for private_key.txt and rpc_url.txt before calling Secrets Manager. That’s the right idea, but the implementation is incomplete.

What SNX did:

– getWallet and getRpcUrl read the file in /tmp; if it exists, they use it. Otherwise, they fall back to Secrets Manager.

Remaining problems:

1. The initial fs.readFile isn’t wrapped in try-catch. On the first cold start, the file won’t exist, fs throws ENOENT, and the whole invocation crashes before Secrets Manager is even reached.
2. When SNX does fetch the secret, it never writes it back to /tmp, so every new container hits Secrets Manager again. This keeps the latency and the extra API charge that was supposed to be avoided.

Why this matters:

A cold-start crash means the keeper skips a cycle. Repeated Secrets Manager calls add 100ms+ to cold starts and burn through the free tier.

Wrap fs.readFile in a try-catch that suppresses only “file not found.” After fetching the secret, immediately write it to /tmp with permissions 0600 so warm invocations can hit the cache. Maintain a module-level variable to prevent repeated file system access in the same invocation.

Ensure the file written to /tmp is encrypted.

A custom IAM role was introduced (good) but granted secretsmanager:GetSecretValue on *. If an attacker gets Lambda code-exec they can read any secret in the account.

Scope the policy to the two specific secret ARNs (private key and RPC URL) or use a dedicated Secrets Manager resource policy.

The LPMigration.sol contract's migrate function was updated to address a previous vulnerability (High-01: Anyone can update users userLastDeposited). This remediation involved removing the receiver parameter from the migrate function, and instead, the msgSender (derived via ERC2771Context._msgSender()) is now implicitly used as the recipient of the AutoCompoundLP tokens.

However, the ILPMigration.sol interface still defines the migrateLP function with the receiver parameter:

function migrateLP(uint128 accountId, address receiver) external;

This creates a discrepancy between the interface definition and the actual implementation in LPMigration.sol.

To maintain consistency and clarity, the migrateLP function signature in the ILPMigration.sol interface should be updated to match the implemented migrate function in LPMigration.sol.

Also consider inheriting ILPMigration in LPMigration.

Division is still rounded down here

function decayingRedemptionPenalty(
        address user,
        uint256 assets
    ) public view returns (uint256) {

       -----------

        return
        assets.
        mulDiv(timeAdjustedFeePercent,
        PERCENT_PRECISION); <<<<<<<
    }

Consider rounding up for both of above divisions

RewardsDistributionRegistry.getUsdValueOfRewards() returns the value of the available rewards in USDC, not USD (even though the name says Usd). The v3 debt is being subtracted from the returned value. This will result in wrong calculations in case of a USDC depeg because the v3 debt is in USD while the returned value from the function is in USDC.

Consider modifying getUsdValueOfRewards to return the value in USD. If you do that, you should convert the absAmount back to USDC in AutoCompoundLP.totalAssetsStata() before dividing it by the static token rate.

The H-03 finding from the main review was not fixed because it's expected that rewards will always be distributed in synths. However, given that the fix is easy and the impact of the issue would not be negligible if normal tokens were distributed as rewards, it's recommended to fix it, since the other part of code is already written to support this case.

Consider implementing the fix from the original issue.

The Alchemy API key was moved from the AutoCompoundLP.t.sol and the other test files to an env, but it is still visible in the repository commit history.

Consider deprecating this api key

As mentioned in L-10, leaving approvals for the ODOS router poses security risks in case the router is exploited. The team acknowledged the issue, but after that the reward approvals to the ODOS router were changed to uint256.max. This decision makes the situation riskier.

Consider not using max approvals if they are not really needed.

According to the AWS documentation, the handler() function should accept parameters of S3Event and Context types. This was true in the previous version of the code. However, with the latest changes, the handle function was modified to accept a boolean value, which is currently more suited for local dev than for prod use in AWS lambda environment.

Check the documentation and revert back to the old signature in case it's needed.

In M-07, SNX said that SECRET_ARN is just unique identifier used to reference private key, so they are ok with it, but the RPC URL shouldn't be publicly visible and "held separately ". However, the same url is still exposed in config.ts

Ensure to implement the "To-Do", as stated in the comments, and avoid exposing the RPC URL. Keep in mind that it will still be visible in the commit history, hence its better to deprecate this key and use a fresh one.

As a fix for L-25, SNX has added slippage tolerance within its config. However, this value is not being passed when quoting via ODOS:

const requestBody: QuoteRequest = {
  chainId: 8453,
  compact: true,
  inputTokens: rewardTokens,
  outputTokens: [
    {
      tokenAddress: USDC,
      proportion: 1,
    },
  ],
  userAddr: slpAddr,
};

According to ODOS API documentation, slippage can be configured using the slippageLimitPercent field in the request body.

Include the configured slippage tolerance by setting the slippageLimitPercent field in the request body. Note that ODOS supports only a single common slippage configuration for all tokens. (hence consider the most conservative one)

As a fix for L-21, SNX has added a deprecation check. However, it still does not handle status codes returned from assembleOdosQuote.

Consider adding explicit error handling for the assembleOdosQuote call.

L-45 is marked as resolved, however proxy API still forwards raw requests and the Lambda does no schema checks. An internal caller sending malformed JSON could crash the function or cause expensive loops.

Add JSON schema validation (e.g. AJV) and early-return 400 on bad input to keep failures cheap.

ODOS requires all addresses to be checksummed according to their API documentation. However all the addresses aren’t checksummed.

There are several addresses, which are yet to be changed in config.ts:

cbBTC, cbETH, SNX

Replace the non-checksum addresses with checksum addresses :

const SNX = '0x22e6966B799c4D5B13BE962E1D117b56327FDa66';
const cbETH = '0x2Ae3F1Ec7F1F5012CFEab0185bfc7aa3cf0DEc22';
const cbBTC = '0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf';