OpenZeppelin audited the AladdinDAO/fx-protocol-contracts repository at commit 56a47ea.
In scope were the following files:
contracts/
├── core/
│ ├── pool/
│ │ ├── AaveFundingPool.sol
│ │ ├── BasePool.sol
│ │ ├── PoolConstant.sol
│ │ ├── PoolErrors.sol
│ │ ├── PoolStorage.sol
│ │ ├── PositionLogic.sol
│ │ └── TickLogic.sol
│ ├── FlashLoans.sol
│ ├── FxUSDBasePool.sol
│ ├── FxUSDRegeneracy.sol
│ ├── PegKeeper.sol
│ ├── PoolManager.sol
│ ├── ProtocolFees.sol
│ ├── ReservePool.sol
│ └── SavingFxUSD.sol
├── fund/
│ ├── strategy/
│ │ ├── AaveV3Strategy.sol
│ │ └── StrategyBase.sol
│ ├── AssetManagement.sol
│ └── IStrategy.sol
└── price-oracle/
├── BTCDerivativeOracleBase.sol
├── ETHPriceOracle.sol
├── LSDPriceOracleBase.sol
├── SpotPriceOracleBase.sol
├── StETHPriceOracle.sol
├── WBTCPriceOracle.sol
└── interfaces/
├── IPriceOracle.sol
├── ISpotPriceOracle.sol
└── ITwapOracle.sol
The f(x) Protocol allows users to efficiently create leveraged positions. It does this by facilitating the leveraging of positions and their aggregation into easily rebalanceable units. This minimizes the risk of full liquidation as the price of the position's asset fluctuates. These positions are demarcated with fxUSD tokens, which the protocol is designed to peg to USDC, broadening the appeal of the fxUSD token in the wider DeFi market. The protocol is large and encompasses many contracts. As such, given the protocol's size and complexity, its documentation can be consulted for an in-depth review of the mechanics of the system as a whole.
This audit covers the new functionality of the f(x) Protocol's "2.0" upgrade, which can be broadly categorized into three parts:
Core Pool Mechanisms: Found in the core directory, these contracts provide the core functionality for building positions and maintaining the pegged value of the debt tokens (fxUSD).
Price Oracle Infrastructure: The protocol relies on real-time pricing of assets to correctly value positions and maintain the fxUSD-USDC peg. The contracts in the price-oracle directory work to integrate price feeds into the protocol.
Asset Management and Reallocation Infrastructure: While positions are open and collateral is deposited, the protocol aims to earn interest from those funds by depositing them into other DeFi protocols. The contracts in the fund directory aim to make this as efficient and seamless as possible.
At the heart of the protocol is the AaveFundingPool contract. Each of these contracts handles the leveraged positions for a specific collateral asset. Currently, there is one AaveFundingPool contract for WBTC and one for stETH. These contracts handle the accounting for the entire market of positions, organizing them by how collateralized they are.
In addition, these contracts provide entry points for various market actions like creating new positions, adjusting existing ones, rebalancing positions that are losing value, and liquidating positions that are outside the acceptable collateralization range. These entry points are not accessible by all network participants and are controlled by a PoolManager contract.
The PoolManager contract is where the collateral for all the positions of all the pools actually resides. It is in charge of correctly moving tokens into and out of the pools and dispatching market operations to the appropriate AaveFundingPool contract. It offers flashloans for the assets it holds and has an entry point for creating, adjusting, and ending positions. To demarcate the value of each position, fxUSD tokens are minted when positions are opened, with one token representing one dollar of debt.
The fxUSD contract is represented in this scope as the FxUSDRegeneracy contract. And because each position is overcollateralized with borrowed assets, each fxUSD token is overcollateralized as well and is burned when positions are adjusted down via deliberate choice or through market pressure (i.e., rebalance or liquidation).
These rebalances and liquidations come from the FxUSDBasePool contract, known as the stability pool. This contract is generally in charge of many activities that maintain the health of the protocol. If the relative price of fxUSD or USDC becomes too far away from the peg, the stability pool facilitates market operations to buy and sell these tokens to bring the relative price back to one. Fees are deposited into this contract, increasing its value over time.
The other contracts in this core layer include the PegKeeper, which acts as a go-between for the stability pool's market operations, the ReservePool contract which covers any collateral losses from undercollateralized debt, and the SavingFxUSD contract, known as fxSAVE, which is an ERC-4626 "wrapper" around the stability pool that automatically reinvests any accruals. Wrapping stability pool shares into fxSAVE actually involves a two-step process involving an out-of-scope contract known as the gauge.
The f(x) 2.0 price oracle fetches prices from multiple data sources, including Chainlink, Uniswap, Curve, and Balancer, to calculate spot prices and anchor prices. The anchor price is mainly based upon Chainlink's oracle price feed using predefined encodings with the aggregator address, scale, and heartbeat set by the owner role. While the spot prices are fetched from multiple pools, only the minimum price (minPrice) and maximum price (maxPrice) of those prices are taken into consideration.
Both minimum price and maximum price are allowed a deviation of up to maxPriceDeviation threshold from the anchor price. For the WBTC pool, the max deviation is 2%, while for the stETH pool, the max deviation is 1%. If the minimum or maximum price deviation exceeds the threshold, then the anchor price is used instead. The minPrice is used for operating on positions, liquidations, and rebalancing, while the maxPrice is used during redeem operations to avoid arbitrage.
Since the protocol holds large amounts of tokens, the asset management contracts add the functionality to deposit one contract's tokens into other, yield-bearing contracts. The AaveV3Strategy contract deposits tokens into an Aave lending pool while the AssetManagement contract allows integration with these strategies. PoolManager, in turn, inherits the AssetManagement contract, allowing it to earn yield while investors hold their leveraged positions.
The contracts in scope are tightly integrated with each other and depend on correct configuration in order to work correctly. As such, during the course of this audit, the following trust assumptions were made:
AaveFundingPoolThe DEFAULT_ADMIN_ROLE can:
The EMERGENCY_ROLE can:
The PoolManager contract:
operate and redeemrebalance, and liquidate. This can extend to anyone should the stability pool's total value not meet a thresholdAaveV3StrategyThe HARVESTER_ROLE can:
The operator can:
The operator role is intended for contracts like the pool manager and the stability pool that inherit the AssetManagement contract.
FxUSDBasePoolThe DEFAULT_ADMIN_ROLE can:
The ASSET_MANAGER_ROLE can:
The PegKeeper's STABILIZE_ROLE can:
FxUSDRegeneracyThe PoolManager can:
The PegKeeper's BUYBACK_ROLE can
PegKeeperThe BUYBACK_ROLE can:
fxUSDRegeneracy) contractThe STABILIZE_ROLE can:
The DEFAULT_ADMIN_ROLE can:
MultiPathConverter contract (used for) swapping assetsPoolManagerThe DEFAULT_ADMIN_ROLE can:
rebalance and liquidateThe ASSET_MANAGER_ROLE can:
The HARVESTER_ROLE can:
The EMERGENCY_ROLE can:
The stability pool allows anyone to:
ReservePoolThe DEFAULT_ADMIN_ROLE can:
The PoolManager can:
SavingFxUSDThe DEFAULT_ADMIN_ROLE can:
The CLAIM_FOR_ROLE can:
The owner can:
The redeem function in the PoolManager contract of the protocol allows any user to burn fxUSD and receive collateral at a cheaper than market rate in return. The purpose of this function is to prevent depegging scenarios of the fxUSD stablecoin through a disincentive. During the function call, the PoolManager contract calls the redeem function of the BasePool contract to calculate the collateral to be returned. This function starts liquidating from the top tick until the desired amount of rawDebts (fxUSD) is covered via the _liquidateTick function. During this function call, the top tick is always shifted to a new parent node and the old node becomes its child node.
It is important to note that there is no minimum rawDebt (fxUSD to be burned) requirement placed in the redeem function. This allows an attacker to redeem small amounts of fxUSD and create multiple nodes for a tick without shifting it from the top tick and to push the positions of that tick down into a spiral of 100s to 1000s of child nodes.
Additionally, the operate function in the BasePool contract always updates the position to the latest parent node from the current child node before any other calculations. Note that the _getRootNodeAndCompress function which gets the root node for a position, is a recursive function which is easily prone to stack overflow error.
An attacker can leverage the above mentioned liquidate tick design, the missing minimum rawDebt check, and the recursion property to execute the following steps:
Repeatedly call the redeem function with a minimal amount of rawDebt(fxUSD) to burn (For example burning 2 wei around 150 times). This ensures that the top tick is never updated and 150 child nodes are created.
Call the redeem function again with a calculated high amount to shift to new top tick to target more positions.
Once the targeted tick has become the top tick again, repeat step 1.
Due to this mechanism, whenever the user tries to close or update one of those affected positions using the operate function, the _getRootNodeAndCompress will fail with stack overflow error as child nodes are above certain limit. This POC demonstrates the stack overflow behavior by manipulating the top tick and locking the funds of all positions corresponding to that tick. The users won't be able to close or update their positions, they can only be rebalanced or liquidated, thus their funds will get locked.
To address the underlying issue, consider migrating to a non-recursive version of the _getRootNodeAndCompress function. To further prevent gas griefing attacks via the redeem function, consider implementing additional checks such as a minimum rawDebt requirement to ensure that the top tick always moves, which would increase the difficulty for any attacker attempting to target a tick.
Update: Resolved in pull request #22.
The team has migrated to an iterative version of the _getRootNodeAndCompress() function and added a minimum rawDebts requirement for the redeem, rebalance, and liquidate functionalities. An admin function has also been added to compress the node chain in case of any unintended behaviors detected through the off-chain monitoring of the tree structure. The f(x) Protocol team stated:
We have implemented the following changes in response to the observed edge cases:
- Minimum Raw Debt Threshold Added: We have introduced a minimum raw debt requirement for
redeem,rebalance, andliquidateoperations to prevent dust-level abuse and ensure tick movement.- Tick Movement Check: The
redeemfunction now reverts in case the tick does not move, guarding against stale state transitions.- Path Compression Function: We have replaced the recursive
getRootNodeAndCompressfunction with a non-recursive internal version to avoid stack overflow. Apublicadmin version is also available for manually compressing excessively long chains.The above changes improve protocol stability while retaining the ability to monitor and adapt to edge-case scenarios dynamically.
The flashLoan function of the FlashLoans contract uses the returnedAmount < amount + fee condition to validate repayment. However, returnedAmount is computed as the post-callback balance minus the pre-loan balance, which only represents the extra tokens sent to cover fees. As a result, returnedAmount < amount + fee is always true, causing every flash loan to revert unless the borrower somehow returns the entire principal, plus fee, as the fee.
Consider changing the condition to returnedAmount < fee so that the function correctly enforces repayment of the fee.
Update: Resolved in pull request #17. This pull request fixes the issue by calculating prevBalance after the token transfer to the receiver. Additionally, compliance to EIP-3156 standard will be achieved once M-01 is resolved.
The price oracles in the protocol are designed to fetch the prices of the collateral from the Chainlink Data feed which acts as the anchorPrice and also uses multiple on-chain pools to fetch spot prices which act as minPrice and maxPrice of the same. minPrice is derived by taking the lowest of all the spot prices fetched and, similarly, maxPrice is derived by taking the highest of the spot prices fetched.
The getPrice function of the oracle contracts ensures that the minPrice and maxPrice values returned from the on-chain pools have not deviated from the anchorPrice by more than 1%. If the price has deviated by more than 1%, it resets the respective deviated minPrice or maxPrice to anchorPrice. The minPrice of the collateral is used during operating positions, rebalancing, and liquidations. On the other hand, the maxPrice is used in the redeem functionality.
However, despite the minPrice and maxPrice being allowed a deviation of 1%, only one pool is required to manipulate the price. Thus, an attacker can target the pool with the lowest TVL or a pool where manipulation can be possible in the same transaction, and manipulate the price in such a way that it causes the anchorPrice to deviate exactly by 1% and bypass the deviation check.
This manipulation of 1% of the prices allows malicious liquidators to lower the minPrice to force vulnerable positions to get liquidated early and generate more profits. Similarly, maxPrice can be manipulated to deviate by more than 1% and reset to anchorPrice, which opens up arbitrage opportunities during the redeem functionality.
By collapsing all spot prices into one minPrice and one maxPrice, and then forcing any outlier back to the anchor, the intended multi-pool resilience is negated. In practice, the system always falls back to either a single manipulated pool or the anchor feed. In other words, compromising just one low-TVL pool turns the “diversified” oracle into a single point of failure.
For example, the current stETH price oracle depends upon 3 pools for the ETH/USD spot price: WETH/USDC Uniswap V2, WETH/USDC Uniswap V3 0.05%, and WETH/USDC Uniswap V3 0.3%. The attacker only needs to manipulate one of these pools, and given that the TVL of the Uniswap V2 pool currently stands at $19 million, it is easily manageable for an attacker to deviate the price by 1% and generate profit by liquidating high-value positions. The Uniswap V3 pools can also be subject to manipulation by a combination of multiple transactions.
Consider redesigning the price oracle to not be dependent upon a single spot price if possible. Alternatively, consider ensuring that the selected pools, upon which spot prices are fetched, meet the minimum TVL requirement calculated based on the maximum liquidation profits possible and the fees paid to deviate the pool by 1% of the TVL.
Update: Resolved. The WETH/USDC Uniswap V2 pool with low TVL will be removed. In addition, the f(x) Protocol team has implemented sufficient risk-control measures, including an in-house team to monitor the liquidity of all the pools.
The f(x) Protocol team stated:
The core price data used by the f(x) Protocol oracle comes from Chainlink. If the protocol goes down, the f(x) Protocol administrator needs to intervene actively and urgently stop all core functions of the f(x) Protocol, such as
operatepositions,rebalance,redeem, andliquidate, to avoid losses of f(x) Protocol users' assets. When the Chainlink oracle is operating normally, the protocol sets different ranges of deviation thresholds for different quoted assets.When the price fluctuation of the quoted asset does not exceed this range, the current quoted data is kept unchanged, which may cause a small range of price differences between the Chainlink quote and the actual market price. For operations that are very sensitive to price accuracy, such as position liquidations on the f(x) Protocol, a small range of price difference may also cause large positions to lose too much principal. As such, the data provided by Chainlink is not suitable for direct use. In order to deal with the problem of delayed Chainlink price updates, the f(x) Protocol supplements and integrates spot price data from multiple DEXs.
Although spot prices are closer to the real-time market status, they are more likely to be manipulated. To avoid any price manipulations on DEXes, the f(x) Protocol weighs the characteristics of Chainlink's price stability and the timeliness of DEX spot prices and builds a balance mechanism, which forms the current f(x) Protocol oracle quotation rules, namely: the f(x) Protocol selects all DEX spot price data and the anchor price provided by Chainlink to obtain the maximum price data
maxPriceand the minimum price dataminPrice.If the price deviation between the maximum price data
maxPrice(minimum price dataminPrice) and theanchorPricedoes not exceed the preset parametermaxPriceDeviation, the data is considered valid. Otherwise, theanchorPricewill be used as the valid data. Under the current f(x) price acquisition rules, if a malicious attacker manipulates the spot price of a DEX to deviate from the price range of the protocol preset parametermaxPriceDeviation, then f(x) Protocol will eventually use the more credibleanchorPriceas the effective price.Compared with the best price in the current market, the pricing strategy of f(x) Protocol may cause users to lose a very small proportion of their assets when prices are manipulated or when market prices fluctuate greatly. This is similar to the inevitable slippage in AMM and is an acceptable compromise under the premise of ensuring system robustness. On the other hand, this design can greatly reduce the risk of spot price attacks and better protect the security of users' assets. Therefore, on the whole, we believe that the current pricing strategy of f(x) Protocol is in the best interests of users.
Regarding the question "manipulating a specific pool so that its price deviates by exactly 1% from the
anchorPrice, bypassing deviation detection, and creating arbitrage opportunities.", we analyze the functional modules that use these prices in f(x) Protocol (taking stETH assets as an example):Functional modules using
minPrice:operatepositions,rebalance, andliquidateoperations.1.
operatepositions operation:minPriceprice data is only used to determine the value of user collateral when there is over-collateralization. Even if the attacker manipulates the minimum price to make it deviate from theanchorPriceby 1%, due to the existence of the over-collateralization mechanism, the price manipulation behavior of a single transaction will not have any adverse effects on the protocol.2.
rebalanceandliquidateoperations: From the above analysis, we can know that, in theory, it is possible to control theminPriceto make it deviate from theanchorPriceby just 1%, but in practice, attackers need to consider the difficulty and attack cost more. In most cases, f(x) Protocol performsrebalanceandliquidateoperations accompanied by a sharp drop in the price of the collateral asset.First, the spot price of each DEX fluctuates greatly at this time, and the
anchorPricecorresponding to Chainlink also changes accordingly. It is very difficult to accurately manipulate the spot price so that the price difference between it and theanchorPriceis within 1% (ensuring the maximization of potential attack profits).Secondly, even if the attacker happens to construct a matching
minPrice, due to the sharp drop in the price of the collateral asset at this time, there must be a large number of swap transactions in the corresponding pool, so the 1% profit earned by the attacker is likely to be insufficient to cope with slippage, handling fees and other fees, which further compresses the arbitrage space.Finally, the
anchorPriceprovided by Chainlink combines the price data of various protocols or exchanges in the current market and provides the average price of the current market. The price change itself is lagging. When the price of the collateral asset falls, it is very likely that the spot price of some DEXs will differ from theanchorPriceby more than 1%. In this case, if the protocol directly uses theanchorPrice, the attack behavior will be more complicated and require additional costs (pull all pool prices exceeding 1% back to within 1%).In summary, the attacker's method of manipulating the minimum price to make it deviate from the
anchorPriceby 1% faces great difficulty, uncertainty, and high cost, and is difficult to achieve.Functional module using
maxPrice:redeemoperation.3. In the upcoming upgraded version, the
redeemoperation will only be allowed to be executed when the fxUSD token is unpegged. Assuming that the current fxUSD is unpegged, although this situation is rare, the attacker needs to manipulate all spot prices at this time to make the spot prices that exceed theanchorPricefall back to theanchorPricein order to redeem the higher value collateral. Obviously, this attack requires the attacker to pay the cost of controlling multiple pools. From the final result, even if the attacker finally uses theanchorPriceto redeem the collateral assets, the price is still in a reasonable price range in the market, although this price is not the most favorable price for the user's position and there is no significant arbitrage space.Overall, the attacker's solution of manipulating a specific pool to control the spot price and bypass deviation detection to achieve the purpose of arbitrage liquidation is very difficult to implement and requires a high attack cost. In order to deal with oracle price manipulation, f(x) Protocol has introduced a series of risk control measures, from obtaining effective price solutions to real-time monitoring on the chain, such as the following:
- 1. As mentioned in the question, f(x) Protocol selects the pool with good liquidity and top TVL in the current market as the spot price source for different collateral assets.
- 2. For different collateral assets, f(x) Protocol combines the deviation threshold of Chainlink's
anchorPriceand designs different preset parametersmaxPriceDeviation.- 3. When running on the current chain, multiple watchers are added to quickly adjust user positions when the price of the mortgage asset fluctuates greatly, greatly reducing the probability of user positions being liquidated.
Therefore, we believe that the current price mechanism of f(x) Protocol has achieved a reasonable trade-off between anti-manipulation and market reflection, which is in the best interests of users. And, in the meantime, we have an in-house team that is monitoring the liquidity of all pools and will remove pools with small liquidity.
At the end, we would like to present the rationale behind our current oracle design:
- Anchor Price from Chainlink: Serves as the base for all comparisons.
- Deviation Thresholds: Each asset has a max price deviation (e.g., 1% for stETH, 2% for WBTC). If spot prices stay within this deviation from the anchor, they are accepted; otherwise, the system falls back to
anchorPrice.- Spot Price Aggregation: Spot prices are collected from multiple high-liquidity DEX pools to reduce manipulation exposure.
Key Mitigation Points:
- If an attacker manipulates a single DEX pool beyond the deviation threshold, its price is ignored.
- The use of
minPricefor liquidation andmaxPricefor redemption ensures a defensible balance between reactivity and manipulation resistance.redeemis only enabled off-peg, and even then, arbitrage is economically infeasible.- The WETH/USDC Uniswap V2 pool with low TVL has been removed.
- All spot sources are selected for depth and reliability, and additional on-chain watchers assist with fast-moving markets.
We believe that this approach achieves a strong trade-off between market responsiveness and security.
ERC-3156 specifies:
After the callback, the
flashLoanfunction MUST take the amount + fee token from the receiver, or revert if this is not successful.
Instead of taking the token, flashLoan expects the caller to have returned the tokens. This will stop the contract from integrating with any compliant IERC3156FlashBorrower contracts as they will not return the tokens to the contract here.
ERC-3156 further specifies:
The
flashFeefunction MUST return the fee charged for a loan of amount token. If the token is not supportedflashFeeMUST revert.
The use of the word "supported" is ambiguous here, in that it does not specify that a maximum loan amount of zero means that a token is "unsupported." However, the ERC does conflate returning zero in maxFlashLoan to mean that a token is unsupported. Therefore, in situations where maxFlashLoan will return zero, flashFee must revert. Currently, the function simply calculates a fraction of the amount given, regardless of the token.
Consider fixing the flashLoan and flashFee functions to be compliant with ERC-3156 as described above.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
The requestRedeem function of the FxUSDBasePool contract registers a redeem request from a user. Once the redeemCoolDownPeriod period has passed, the user can redeem that amount of fxBASE token and get back fxUSD and USDC as per the ratio in the pool. The motivation for this functionality is to avoid high levels of redemption causing a run on the stability pool.
However, there is no expiry of the redemption request made by the user. A user can deposit in the pool and immediately call the requestRedeem function to register their request to redeem. Afterward, they redeem at any time once the cool-down period has expired. This would make the redemption request functionality useless.
Consider adding an expiration time for the redemption requests. After this time, redeeming should not be allowed and the user should have to request a redemption again.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
The current redemption design prevents deposit and redeem from happening in the same block. As long as the
redeemCoolDownPeriodis non-zero, users must wait before redeeming, which acts as a sufficient deterrent to immediate arbitrage. Therefore, we do not believe any further changes are required at this time.
Each pool in the PoolManager contract has a maximum capacity of collateral that it can hold. The _changePoolCollateral function reverts if the change surpasses the capacity. When a tick has bad debt, the protocol uses funds from the ReservePool contract to cover the uncollateralized part of the debt to be able to facilitate the liquidation.
When a pool is liquidated through the liquidate function of the PoolManager contract, the funds pulled from the reserve are added to the current pool's collateral amount. If the sum of the funds from the reserve and the current balance surpasses the capacity (i.e., capacity<bonusFromReserve+balancecapacity < bonusFromReserve + balancecapacity<bonusFromReserve+balance), the liquidation will fail.
Consider adding the reserve's funds to the pool collateral variables after the liquidation collateral has been subtracted.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
The case is rare, and given the low likelihood of it occurring in practice, we have opted not to make any changes at this time. The current implementation maintains simpler and more predictable behavior, and modifying the reserve collateral logic could introduce unnecessary complexity for an edge case that has minimal user impact.
totalStableToken Used in Stability PoolThe sync modifier updates the totalStableToken variable to the latest value in case the stable token has been deposited into a strategy and is generating yield. All external functionalities such as deposit, redeem, etc. update this totalStableToken variable using the sync modifier before proceeding further.
However, the previewDeposit, previewRedeem, and nav functions of the FxUSDStabilityPool contract use a stale value of totalStableToken variable, which could lead to incorrect return values from these view functions. For example, to protect against inflation attacks, previewDeposit could end up returning more shares than what the user had specified as minSharesOut during deposit, causing it to revert.
Consider modifying the functions to calculate the latest value of totalStableToken.
Update: Resolved in pull request #21.
fxSAVE When Total Supply Is ZeroThe ERC-4626 Vault contract uses the _decimalsOffset function to add more precision to share values. The issue is that the value returned by _decimalsOffset is 0 if the underlying token has 18 decimals (which is the case for most tokens, including fxBASE).
Since the totalAssets function is dependent upon balanceOf(address(this)), when totalSupply is 0, an attacker can mint 10 shares for 10 wei of fxBASE, and then donate 100e18 fxBASE tokens directly to the contract to inflate the price-per-share of fxSAVE. When a user deposits 1e18 worth of shares, they will get 0 shares in return, whereas the attacker may only lose 1e17 shares. It can be observed that the attacker can lock 1e18 of a user's assets at the cost of ~1e17 fxBASE tokens.
Since the pool is already deployed, the likelihood of this attack vector materializing is quite low. Nonetheless, consider sending some fxSAVE tokens to a dead address to ensure that the totalSupply of fxSAVE is never 0.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
This issue was only relevant during the launch phase. Our production deployment of the fxSAVE token included a guarded launch mechanism which ensured that the total supply was never zero at runtime. Therefore, we consider this issue effectively mitigated in practice.
The getPrice() function of ETHPriceOracle, the getPrice() function of LSDPriceOracleBase, the getPrice() and getExchangePrice() functions of BTCDerivativeOracleBase, all calculate the minimum price deviation by using the (anchorPrice - minPrice) / minPrice > maxDeviation formula. If the deviation is higher than the max deviation allowed, the minimum price is reset to the anchor price.
However, this calculation is incorrect. It checks the deviation from minPrice which makes deviation restrictive and causes it to always be less than the max deviation allowed from the anchor price. The correct formula is to check the deviation against anchorPrice instead of minPrice: (anchorPrice - minPrice) / anchorPrice > maxDeviation.
When calculating the minimum price deviation, consider checking the deviation against anchorPrice instead of minPrice.
Update: Resolved in pull request #20.
The operate function in PoolManager contract allows any user to open, close or update their positions. If a user wants to close their position, they can simply use the minimum amount of int256 (i.e. type(int256).min parameter for newDebt and newColl) while providing their positionId parameter. If the positionId parameter is 0 then it's considered a new position to be opened.
However, it is observed that users that have no positions created, can call the operate function with the type(int256).min parameter for both newDebt and newColl with positionId = 0 and mint null positions with latest positionId. This could create ambiguity for off-chain analysis due to event spamming. This is not an ideal behavior as protocol fees are also not charged for opening such positions.
Consider adding a further check in the operate function to avoid null positions being minted.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
We acknowledge the issue where users can trigger the operate function with
type(int256).minvalues for bothnewDebtandnewCollwithpositionId = 0, resulting in a “null” position being minted. While this behavior does not pose any functional or economic risk to the protocol (no collateral, no debt, no system impact), we agree it may cause off-chain event noise or indexing ambiguity.Resolution Approach:
The current implementation does not charge protocol fees for zero-value operations.
To maintain clean off-chain indexing and avoid unnecessary event spam, we plan to implement a filter that rejects no-op state transitions (i.e., zero collateral and zero debt for new positions) in a future release.
_This is a non-critical UX-level issue with no impact on funds or protocol logic, but we value the feedback and will improve clarity for integrators and indexers.
If an L2 sequencer goes offline, users will lose access to read/write APIs, effectively rendering applications on the L2 network unusable—unless they interact directly through L1 optimistic rollup contracts.
While the L2 itself may still be operational, continuing to serve applications in this state would be unfair, as only a small subset of users could interact with them. To prevent this, Chainlink recommends integrating their Sequencer Uptime Feeds into any project deployed on an L2. These feeds help detect sequencer downtime, allowing applications to respond appropriately.
Several oracle calls in the codebase may return inaccurate data during sequencer downtime, including:
AggregatorV3Interface(aggregator).latestRoundData call in FxUSDBasePool.solAggregatorV3Interface(aggregator).latestRoundData call in SpotPriceOracleBase.solTo help your applications while deploying on Base chain identify when the sequencer is unavailable, you can use a data feed that tracks the last known status of the sequencer at a given point in time.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
The alloc function of the AssetManagement contract does not verify whether the strategy supports the specified token. In the case of AaveV3Strategy, any unknown tokens sent to it will not be recoverable because withdraw() and kill() can only interact with the token specified in ASSET.
Consider checking whether the strategy's ASSET is the same as the asset in alloc().
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
Strategy allocation is currently controlled by a multisig governance process. Execution is carefully reviewed before approval. Looking forward, as the protocol transitions to fully on-chain governance, we will enhance strategy validation logic to make such failures structurally impossible.
In the alloc function of the AssetManagement contract, there is no validation whether the new strategy address is valid and supports kill() function. Once an incorrect address is set, both the kill() function and alloc() function to update the strategy will revert due to this Strategy.kill() not being supported.
Consider adding proper validation on added strategy addresses during alloc() function or allow the replacement of strategies without a call to kill.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
This issue shares the same category as L-02. Currently, updates to strategy allocation are gated by multisig and are reviewed carefully before execution. In the future, on-chain governance enhancements will introduce stricter validation logic, improving upgrade safety and modularity.
In the operate function of BasePool contract, the newRawDebt value is required to be greater than MIN_DEBT which is 1e9. newRawDebt is then converted to debt shares, which divides the rawDebt with debtIndex, resulting in a value that is less than 1e9 minimum debt shares requirement.
However, while adding this position to a tick, the function again checks for MIN_DEBT requirement of 1e9 against the debtShares, which will revert the function call. Hence, users will not be able to open a position with an amount greater than MIN_DEBT until the converted debtShares are strictly higher than 1e9. Thus, as time passes, debtIndex will increase, causing the actual minimum debt requirement to increase as well.
Consider converting MIN_DEBT into a MIN_SHARES requirement that uses the debt index in _addPositionToTick. Alternatively, consider checking for the MIN_DEBT requirement immediately before the call to _addPositionToTick with the rawDebts variable is made, and removing the check within the function.
Update: Acknowledged, not resolved. The f(x) Protocol team stated:
We acknowledge the observation. The current
MIN_DEBTcheck is intentionally minimal and only used to avoid edge-case tick computation errors. Based on its functional intent and low impact, we believe that the current implementation is sufficient.
In order to clearly identify the Solidity version with which the contracts will be compiled, pragma directives should be fixed and consistent across file imports.
No file in scope uses a fixed Solidity version and many of them differ in the versions they use. As such, consider using the same, fixed pragma version in all the files.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
_takeAccumulatedPoolFeeThe _takeAccumulatedPoolFee function of the ProtocolFees contract returns a fees variable that is overwritten three times: first by accumulatedPoolOpenFees, then by accumulatedPoolCloseFees, and finally by accumulatedPoolMiscFees. As a result, the returned value only reflects the last category. In addition, no internal or external caller ever uses this return value, making it meaningless and potentially confusing.
Consider removing the unused return value to improve the clarity and maintainability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
The updateOnchainSpotEncodings setter function should validate its inputs. However, the non‑empty check is only performed in some oracles:
BTCDerivativeOracleBase.updateOnchainSpotEncodings reverts when prices.length == 0.ETHPriceOracle.updateOnchainSpotEncodings has no check on prices.length.LSDPriceOracleBase.updateOnchainSpotEncodings applies the check only for one spotType.This inconsistency allows empty or malformed encodings to be set, which can lead to read functions reverting or returning invalid data.
Consider adding a uniform sanity check in every updateOnchainSpotEncodings implementation to ensure data integrity across all oracles.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
When a setter function does not check if the value being set is different from the existing one, it becomes possible to set the same value repeatedly, creating the possibility for event spamming. Repeated emission of identical events can also confuse off-chain clients.
Throughout the codebase, multiple instances of such possibilities were identified:
_updateRedeemCoolDownPeriod sets the redeemCoolDownPeriod and emits an event without checking if the value has changed._updateInstantRedeemFeeRatio sets the instantRedeemFeeRatio and emits an event without checking if the value has changed._updateConverter sets the converter and emits an event without checking if the value has changed._updateCurvePool sets the curvePool and emits an event without checking if the value has changed._updatePriceThreshold sets the priceThreshold and emits an event without checking if the value has changed._updateThreshold sets the permissionedLiquidationThreshold and emits an event without checking if the value has changed._updatePriceOracle sets the priceOracle and emits an event without checking if the value has changed._updateTreasury sets the treasury and emits an event without checking if the value has changed._updateOpenRevenuePool sets the openRevenuePool and emits an event without checking if the value has changed._updateCloseRevenuePool sets the closeRevenuePool and emits an event without checking if the value has changed._updateMiscRevenuePool sets the miscRevenuePool and emits an event without checking if the value has changed._updateReservePool sets the reservePool and emits an event without checking if the value has changed.Consider adding a check that reverts the transaction if the value being set is the same as the existing one.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
The ProtocolFees contract inherits the PausableUpgradeable contract, but does not implement any pause functionality. ProtocolFees is then inherited by the FlashLoans and PoolManager contracts that do implement pause functionality. Furthermore, throughout the codebase, it appears that the intention of the developers was to inherit interfaces for implementing contracts. For example, AaveFundingPool inherits IAaveFundingPool and IPool, ETHPriceOracle inherits IPriceOracle, etc. However, the AaveV3Strategy contract does not inherit the IStrategy interface and thus breaks the apparent convention.
Consider having StrategyBase inherit IStrategy and having FlashLoans inherit PausableUpgradeable while removing it from ProtocolFees.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Within the codebase, multiple instances of contracts utilizing gap variables were identified:
Gap variables allow inherited contracts to expand their storage in the future without it colliding with the storage of the inheriting contracts. Since the codebase is already in production, it is important to ensure that any storage changes are reflected in the gap variables.
To better mitigate the risk of storage collision in upgradeable contracts for new deployments, consider utilizing namespace storage or using the custom storage layout that became available in Solitidy version 0.8.29.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the theoretical risk of future storage layout collision due to unstructured gap variables. However, the affected contracts are already deployed, and altering the storage layout would pose significant risk to data integrity and contract behavior. Going forward, we plan to adopt name-spaced storage techniques or structured layout support introduced in Solidity 0.8.29+ to mitigate such risks in future iterations.
PegKeeper sets a storage variable context when its buyback and stabilize functions are called and restores it when those calls are done. The contract uses this variable to ensure that the onSwap function is being called in the context of the other functions. This is precisely one of the use cases for transient storage, which became available in Solidity in version 0.8.24.
In order to save gas on these calls, consider setting the context in transient storage rather than in permanent storage.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of missing docstrings were identified:
AaveFundingPool contract should have NatSpec comments above it.AaveFundingPool.sol, the initialize functionAaveV3Strategy.sol, the POOL, INCENTIVE, ASSET, ATOKEN, and principal state variables along with the totalSupply, deposit, withdraw, and kill functionsAssetManagement.sol, the ASSET_MANAGER_ROLE and allocations state variables along with the kill, alloc, and manage functions.FxUSDBasePool.sol, the initialize and updateInstantRedeemFeeRatio functionsFxUSDRegeneracy.sol, the initialize and initializeV2 functionsIStrategy.sol, the totalSupply, deposit, withdraw, kill, and harvest functionsPegKeeper.sol, the initialize functionPoolManager.sol, the initialize and initializeV2 functionsReservePool.sol, the receive functionSavingFxUSD.sol, the execute and initialize functionsStrategyBase.sol, the HARVESTER_ROLE and operator state variables, as well as the harvest and execute functionsConsider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of incomplete docstrings were identified:
BTCDerivativeOracleBase.sol, the isRedeem parameter of the getBTCDerivativeUSDAnchorPrice function is not documented.PoolManager.sol, the collateralCapacity and debtCapacity parameters of the registerPool function are not documented.ReservePool.sol, the amount parameter of the withdrawFund function is not documented.Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Several storage slots are documented in a way that could lead to confusion. For example, in the ProtocolFees contract, the comments above _miscData show the components of the slot being laid out with the zero index on the left whereas, the prevailing convention is to show slots with the zero index on the right. This confusion is compounded by the designation of the left side as the most significant bits (MSB), which is correct under the standard convention but incorrect if the layout is reversed.
Throughout the codebase, multiple instances of such misleading storage-slot descriptions were identified:
miscData, rebalanceRatioData, indexData, shareData, and positionMetadata in the PoolStorage contract_miscData in the ProtocolFees contractfundingMiscData in the AaveFundingPool contractTo avoid confusion, consider either reversing the storage depiction so that index 0 starts on the right (aligning with Solidity conventions) or swapping the positions of the most and least significant bits in the comments to match the current left-to-right depiction.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of functions updating the state without an event emission were identified:
AssetManagement.solBTCDerivativeOracleBase.solLSDPriceOracleBase.solConsider emitting events whenever state changes are performed in these functions for improved transparency and better monitoring capabilities.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
_transferOutThe _transferOut function of the AssetManagement contract is meant to transfer a precise amount of assets to the receiver, falling back to the associated strategy if the contract holds insufficient balance. It first sends its on‑hand tokens and then calls the strategy’s withdraw function for the shortfall.
However, in the AaveV3Strategy contract, the withdraw function does not revert when asked to withdraw more than the strategy’s available liquidity. Instead, it simply withdraws as much as possible. As a result, _transferOut may complete without error while transferring less than the intended amount, violating the expectation that it either succeeds fully or reverts.
Consider adding a post‑withdraw sanity check in the _transferOut function to verify that the full amount was transferred and reverting otherwise.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Through the codebase, multiple instances of incorrect or inaccurate comments were identified:
“The price is valid iff |maxPrice‑minPrice|/minPrice < maxPriceDeviation” in BTCDerivativeOracleBase, ETHPriceOracle, and LSDPriceOracleBase does not align the code, which actually uses < 2 * maxPriceDeviation.LSDPriceOracleBase, a comment refers to the LSD/ETH pair, whereas, it should state LSD/USD.Consider correcting the above-mentioned comments to improve the clarity and readability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
In the SavingFxUSD.sol file, multiple instances of hardcoded addresses were identified:
0xAffe966B27ba3E4Ebb8A0eC124C7b7019CC762f8 value0x365AccFCa291e7D3914637ABf1F7635dB165Bb09 valueConsider declaring hardcoded addresses as immutable variables and initializing them through constructor arguments. This allows code to remain the same across deployments on different networks and mitigates situations where contracts need to be redeployed due to having incorrect hardcoded addresses.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
calldata Instead of memoryWhen dealing with the parameters of external functions, it is more gas-efficient to read their arguments directly from calldata instead of storing them to memory. calldata is a read-only region of memory that contains the arguments of incoming external function calls. This makes using calldata as the data location for such parameters cheaper and more efficient compared to memory. Thus, using calldata in such situations will generally save gas and improve the performance of a smart contract.
Throughout the codebase, multiple instances where function parameters should use calldata instead of memory were identified:
AaveFundingPool.sol, the name_ parameterAaveFundingPool.sol, the symbol_ parameterBTCDerivativeOracleBase.sol, the encodings parameterETHPriceOracle.sol, the encodings parameterFxUSDBasePool.sol, the _name parameterFxUSDBasePool.sol, the _symbol parameterFxUSDRegeneracy.sol, the _name parameterFxUSDRegeneracy.sol, the _symbol parameterFxUSDRegeneracy.sol, the _minOuts parameterLSDPriceOracleBase.sol, the encodings parameterProtocolFees.sol, the pools parameterSavingFxUSD.sol, the params parameterConsider using calldata as the data location for the parameters of external functions to optimize gas usage.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.
Throughout the codebase, instances of revert were identified in the following contracts:
| Contract | Instances |
|---|---|
FxUSDBasePool |
2 |
FxUSDRegeneracy |
4 |
SavingFxUSD |
1 |
AssetManagement |
2 |
StrategyBase |
1 |
BTCDerivativeOracleBase |
1 |
SpotPriceOracleBase |
2 |
Many of these instances were reverts with non-descriptive messages. For conciseness, clarity, and gas savings, consider replacing these revert messages with custom errors.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Within ProtocolFees.sol, multiple instances of unnecessary casts were identified:
uint256(newRatio) castuint256(newRatio) castuint256(newRatio) castuint256(newRatio) castuint256(newRatio) castuint256(newRatio) castTo improve the overall clarity and intent of the codebase, consider removing any unnecessary casts.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, the majority of the scoped contracts have an inconsistent ordering of functions.
AaveFundingPool contract in AaveFundingPool.solAaveV3Strategy contract in AaveV3Strategy.solAssetManagement contract in AssetManagement.solBTCDerivativeOracleBase contract in BTCDerivativeOracleBase.solBasePool contract in BasePool.solETHPriceOracle contract in ETHPriceOracle.solFlashLoans contract in FlashLoans.solFxUSDBasePool contract in FxUSDBasePool.solFxUSDRegeneracy contract in FxUSDRegeneracy.solLSDPriceOracleBase contract in LSDPriceOracleBase.solPegKeeper contract in PegKeeper.solPoolManager contract in PoolManager.solPoolStorage contract in PoolStorage.solPositionLogic contract in PositionLogic.solProtocolFees contract in ProtocolFees.solReservePool contract in ReservePool.solSavingFxUSD contract in SavingFxUSD.solSpotPriceOracleBase contract in SpotPriceOracleBase.solStrategyBase contract in StrategyBase.solWBTCPriceOracle contract in WBTCPriceOracle.solTo improve the project's overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide's Layout and Order of Functions.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
++i) Can Save Gas in LoopsThroughout the codebase, multiple opportunities for optimizing loop iteration were identified:
Consider using the prefix-increment operator (++i) instead of the postfix-increment operator (i++) in order to save gas. This optimization skips storing the value before the incremental operation, as the return value of the expression is ignored.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of unused imports were identified
import { ITwapOracle } from "./interfaces/ITwapOracle.sol"; imports unused alias ITwapOracle in ETHPriceOracle.solimport { ITwapOracle } from "./interfaces/ITwapOracle.sol"; imports unused alias ITwapOracle in LSDPriceOracleBase.solConsider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of state variables lacking an explicitly declared visibility were identified:
fxSAVE state variablespotPriceOracle state variableFor improved code clarity, consider always explicitly declaring the visibility of state variables, even when the default visibility matches the intended visibility.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as that function's output. They are an alternative to explicit in-line return statements.
Throughout the codebase, multiple instances of unused named return variables were identified:
AaveFundingPool.sol, the ratio and step return variables of the getOpenRatio functionPositionLogic.sol, the debtRatio return variable of the getPositionDebtRatio functionConsider removing these unused named return variables unless they should be used.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of unused errors were identified:
ErrorRebalanceOnLiquidatablePosition error in PoolErrors.solErrorInsufficientCollateralToLiquidate error in PoolErrors.solErrorRatioTooLarge error in ReservePool.solErrorRebalancePoolAlreadyAdded error in ReservePool.solErrorRebalancePoolNotAdded error in ReservePool.solTo improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused errors.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
msg.senderA contract may use the _msgSender and _msgData functions in certain cases where they allow meta transactions and have overridden these methods to extract the original message sender/data. Consistent use of _msgSender/msg.sender and _msgData/msg.data within a contract should be manually checked. This is because any inconsistency may be an error and could have unintended consequences for executing meta transactions.
In the onlyOperator modifier of the StrategyBase contract, msg.sender is being used instead of _msgSender.
Consider manually checking for any inconsistent usage of msg.sender and msg.data, and updating such instances to follow a consistent behavior throughout the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of unused constants were identified:
AaveFundingPool.sol, the INTEREST_RATE_OFFSET constantAaveFundingPool.sol, the TIMESTAMP_OFFSET constantPoolConstant.sol, the X60 constantPoolConstant.sol, the X96 constantTo improve the overall clarity and intent of the codebase, consider removing unused constants.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple instances of literal numbers being used directly were identified:
50000000000000000 literal number in AaveFundingPool.sol500000000000000000 literal number in BasePool.sol995000000000000000 literal number in PegKeeper.solFor literal numbers with this many digits, consider using Ether Suffix, Time Suffix, or Scientific Notation. This will help improve readability and prevent misleading code that could have unintended consequences.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
In the initialize function of the AaveFundingPool contract, a literal value (1e9) with unexplained meaning is being used.
Consider defining and using constant variables instead of using literals to improve the readability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Consider adding a NatSpec comment containing a security contact at the top of each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Throughout the codebase, multiple instances of mappings without named parameters were identified:
allocations state variable in the AssetManagement contractredeemRequests state variable in the FxUSDBasePool contractmarkets state variable in the FxUSDRegeneracy contractpoolInfo, rewardSplitter, and tokenRates state variables in the PoolManager contractpositionData, positionMetadata, tickBitmap, tickData, and tickTreeData state variables in the PoolStorage contractaccumulatedPoolOpenFees, accumulatedPoolCloseFees, and accumulatedPoolMiscFees state variables in the ProtocolFees contractlockedProxy state variable in the SavingFxUSD contractConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Within the SavingFxUSD.sol, there are two contract declarations.
Consider separating the LockFxSaveProxy contract into its own file to make the codebase easier to understand and maintain.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
Throughout the codebase, multiple opportunities for improving code readability were identified:
onlyFxSave modifier of the PoolManager contract ensures that the caller is the fxBase contract, not the fxSAVE contract. As such, consider renaming the modifier to onlyFxBase.totalSupply function of the AaveV3Strategy contract returns the balanceOf of AToken which translates to "total deposited assets + yield generated" as this function is later used to check the total managed assets of the strategy in the AssetManagement contract. Consider renaming the function so that its name matches its behavior and does not create confusion while adding more strategies in the future.manage function of the AssetManagement contract only acts as a deposit function. Consider renaming it to deposit.Consider implementing the above-given renaming suggestions to improve the readability and maintainability of the codebase.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
In TickLogic, the _getTick function spends a line computing a new ratio that it will never use. Consider removing this superfluous line.
Update: Acknowledged, resolution planned. The f(x) Protocol team stated:
We acknowledge the issue and plan to address it in a future update.
The f(x) Protocol v2.0 introduces a novel stablecoin implementation—fxUSD—with improved stabilization dynamics and an advanced peg-keeping mechanism, alongside its decentralized trading platform, xPosition. The protocol relies on complex mechanisms to maintain stability using batch rebalancing, liquidation, redemption, and a "tick"-based approach for maintaining leveraged positions for each base pool.
It utilizes special stability pools, a peg keeper, funding fees, and a harvesting mechanism to reward the maintainers of the system and mitigate the risks of depegging. The system uniquely manages xPositions with a tick-based approach that groups positions into price bands of roughly 0.15% to improve efficiency while updating the positions. Multiple sources are used to determine the price of the underlying collateral token, including off-chain oracles and a combination of on-chain spot prices.
Certain integrations with the f(x) Protocol such as Gauge, Convex Vault, SpotPriceOracle, RevenuePool, Treasury, fTokens, and MarketV2 were out of scope for this audit. In addition, the protocol largely anticipates funding user positions via external flashloan providers such as Morpho and Balancer using periphery facet contracts which were also out of scope for this audit.
Throughout the audit, the primary focus was on validating the tick logic and various edge cases regarding rebalancing, liquidation, and redemptions, while also assessing the overall economic stability of the system. The system, despite being dependent upon multiple contracts, reflected robust architecture and high resilience.
One critical-severity issue, two high-severity issues and multiple medium- and low-severity issues were identified. The critical-severity issue manipulated tick and node logic to block operate functionality. Of the high-severity issues, one was the susceptibility of the pricing scheme to a single, manipulable liquidity pool which could result in early liquidations of positions. The other one completely blocked the flashloan functionality of the protocol.
Collaboration with the f(x) Protocol team was smooth and highly effective. Their responsiveness and contextual clarity were instrumental in understanding the protocol and the broader reasoning behind the design choices.