This security assessment was prepared by OpenZeppelin.
We audited the OriginProtocol/origin-dollar repository at the eb11498c376b65696c90981757221b076d6226aa commit. In scope were the following contracts:
In scope were the following contracts:
contracts
└── strategies
└── balancer
├── balancerMetaPoolStrategy.sol
├── BaseAuraStrategy.sol
└── BaseBalancerStrategy.sol Origin Ether (OETH) is an ERC-20-compliant token collateralized by Rocket Pool ETH, Lido Staked ETH, Frax ETH and Wrapped ETH. Users can mint OETH by depositing any of these assets in the system's vault to later be invested into different strategies, such as Curve pools.
The audited commit introduces the Balancer MetaPool strategy. This new strategy for generating yield takes advantage of the Aura Finance protocol to boost the yield on collateral deposits to a Balancer MetaPool.
When depositing collateral to Balancer pools, the strategy receives LP tokens called Balancer Pool Tokens (BPTs), which represent the share of the liquidity pool that is provided by the strategy. On top of earning trading fees, Balancer LPs are also rewarded with BAL tokens, proportionally to the amount of liquidity they provided. In order to boost yield, the BalancerMetaPoolStrategy deposits the BPTs to Aura, which results in earning more BAL rewards, as well as AURA tokens. Naturally, the process is reversed when the strategy needs to withdraw collateral to the vault.
The BaseBalancerStrategy contract implements generic logic around Balancer pools, while the BaseAuraStrategy contract adds logic to deposit and withdraw BPTs to Aura pools, as well as collect rewards.
The BalancerMetaPoolStrategy contract implements logic specific to Balancer MetaPools, which are pools geared towards highly correlated but not completely equivalent assets, like the ones used as collateral by the Vault (wEth, rETH, stETH, and frxETH).
To make the strategy compatible with the OETH Vault, the BaseBalancerStrategy contract inherits from InitializableAbstractStrategy.
While the strategy supports rebasing tokens such as stEth and frxEth, Balancer pools do not, so additional wrapping and unwrapping logic is required when depositing and withdrawing liquidity from Balancer. Note that it is intended for the strategy to not necessarily support all pool assets, and it is expected to only deposit/withdraw a subset of those. We will use "strategy assets" for the assets supported by the strategy, and "pool assets" for the assets supported by the pool, in their wrapped versions.
Note that the strategy was reviewed with Balancer MetaStable pools in mind, and the current shared functionality such as pricing the BPT tokens or estimating the strategy's underlying value may need to be updated when working with other types of Balancer pools.
Additional assumptions as to the previous audits are as follows:
BalancerErrors and VaultReentrancyLib libraries are part of Balancer's codebase, and as such they are out of scope for this audit and are expected to work correctly.wstEth and sfrxEth contracts are given 1e50 units of allowance to pull stEth and frxEth for wrapping purposes.Update: Because of a change during the fix review process, the calculation of BPT amounts during withdrawals/deposits now assumes that frxETH and stETH are pegged to ETH.
maxWithdrawalSlippage and the maxDepositSlippage.safeApproveAllTokens Implementation Differs From DocumentationThe safeApproveAllTokens function comments specify that the function should approve the Balancer pool to transfer all supported assets from the strategy, as well as approve the wstEth and sfrxEth contracts to pull stEth and frxEth from the Strategy for wrapping purposes. However, the function does not perform the necessary approvals to wrap the tokens, and additionally, it approves the Balancer vault and Aura rewards pool to withdraw an unlimited number of BPTs.
Consider correcting the safeApproveAllTokens function to follow its intended behavior.
Update: Resolved in pull request #1776.
withdrawAll Function Can Withdraw Unsupported TokensIn the BalancerMetaPoolStrategy, the withdrawAll implementation retrieves the pool tokens and iterates through them, calculating the expected minimum amounts out. These calculated amounts are used for exiting the pool, where tokens are received in exchange for BPTs.
While these calculations are correct, if the intention is to withdraw across all tokens supported by the pool, they are incorrect when the assets supported by the strategy are a subset of the assets in the Balancer pool.
During initialization, the BaseBalancerStrategy passes a list of tokens to the InitializableAbstractStrategy, which in turn populates the assetsMapped array. This array contains the strategy's supported assets and is not guaranteed to include all assets supported by the associated Balancer pool.
When exiting, consider only withdrawing assets that are supported by the strategy to prevent ending up with assets that are not accounted for and could potentially become stuck.
Update: Resolved. The Origin team stated:
We decided not to make any changes. While we agree with the identified problem, our strategy currently does not support a configuration where only a subset of the pool's assets can be supported.
getBPTExpected Calculation Is Not RobustThe getBPTExpected functions are used to calculate the minimum amount of BPTs to receive when depositing liquidity, or the maximum amount of BPTs to provide when withdrawing liquidity. They calculate the underlying ETH value of the assets to deposit/withdraw, and divide it by the price of a BPT in terms of the pool's underlying base asset.
There are two issues with this:
stETH or frxETH depegging from ETH would make this untrue, resulting in an overestimation of the amount of BPTs.Overestimating the BPT amount can result in reverts during deposits while underestimating them can result in reverts during withdrawals. Consider accurately calculating the exact number of BPTs required for these operations using Balancer's StableMath.
Update: Resolved in pull request #1795. The Origin team identified the additional concern of MEV attacks and stated:
We have decided to move the primary responsibility of defending against MEV attacks back to the
ValueValueCheckeras our other strategies do. Only the strategist can deposit/withdraw funds and fork tests have been added confirming that theVaultValuechecker used by the strategist catches MEV attack attempts. The checks in deposit/withdraw are now secondary sanity checks to prevent large losses and work without oracles. We can repay our debts in depegged coins, so we are not in danger of insolvency even if the pool mix changes. We did not implement the suggested math that would copy the math that the Balancer pool does under the hood since it would not protect the protocol against MEV sandwich attacks.
The current test suite does not cover all the functionality in the BalancerMetaPoolStrategy.
deposit, only for depositAll. This can be problematic, as supplying only one token at a time will incur more slippage and potentially return fewer BPTs than expected, as opposed to joining the Balancer pool with multiple supported assets at the same time.safeApproveAllTokens.BAL and AURA tokens are actually rewarded when calling collectRewardTokens.Additionally, because of the significant number of external dependencies, there are multiple corner cases that are not covered in the existing test suite:
withdrawAll is called when supported strategy assets are a subset of the pool's assetsmaxDepositSlippage and maxWithdrawalSlippage should be in order to prevent DoS while depositing and withdrawingAdditional tests would not only help maintain the correctness of the codebase when changes are implemented but would also serve as documentation for developers and auditors, aiding them in understanding multiple possible scenarios. Consider increasing the test coverage, especially around scenarios that deviate from normal operation.
Update: Resolved in pull request #1780. The Origin team stated:
1. The
deposittest has not been added because single-asset deposits have been disabled.
2. The test forsafeApproveAllTokenshas been added.
3. The BAL and AURA harvesting test has been added.
4.maxDepositSlippageand how it affects oracle prices: We are no longer using oracles so this portion is skipped. ChangingmaxDepositSlippage(nowmaxDepositDeviation) is tested in H-03. Also, themaxDepositDeviationis much more lenient now asVaultValueCheckerhas become the line of defense against MEV.
5. Whether the underlying value of the Strategy will be reported correctly, irrespective of the liquidity pool being balanced: initial test added in H-03's pull request and enhanced via a separate commit.
6. Stale oracle prices test: skipped since we do not use oracles.
7.withdrawAllon a subset of assets: no test since this functionality is not yet supported.
The _withdraw function redeems BPTs from the Balancer pool in exchange for exact _amounts of tokens, which are then sent to the Vault. This is not a straightforward process, and involves at least two steps which introduce precision loss, potentially reverting the function upon safeTransfer.
wstETH and sfrxETH smart contracts), this method of calculation might also result in slightly fewer units of the required strategy asset.Consider adding a fuzz test to have a greater certainty that rounding errors will not result in a denial-of-service of the _withdraw function, or requesting an error-adjusted amount of liquidity when exiting the Balancer pool.
Update: Acknowledged, will resolve in pull request #1801. The Origin team stated:
The pull request adds more fork test cases. We have fuzzing test infrastructure in development. Once that is done, we will add proper fuzzing tests, as suggested.
Throughout the codebase, there are multiple sections which are difficult to understand, and would benefit from additional or clearer documentation:
getBPTExpected functions.Additionally, while there was documentation around the addition of strategies using Balancer, it was at times misleading as it was not specifically targeted towards the BalancerMetaPoolStrategy, and hence would differ from the implementation that was audited. Consider creating documentation specific to the strategy at hand, explicitly and concisely explaining the design decisions behind each important piece.
Update: Resolved in pull request #1781 and pull request #1795.
In the Balancer documentation, there is a section on risks which arise when using the platform. One section goes into detail about the potential loss of funds when entering and exiting pools.
#### Due to high price impact
When joining a pool, LPs should be aware of the price impact from adding tokens to the pool. In general, adding liquidity in proportional amounts to the token weights of the pool incur low price impact. Adding custom token amounts (non-proportionally) causes the internal prices of the pool to change, as if you were swapping tokens. The higher the price impact the more you'll spend in swap fees.
In the BalancerMetaPoolStrategy, there are multiple deposit function signatures, one of which deposits a single asset adding liquidity to the pool in a non-proportional amount. This function is integrated with the VaultCore via the allocate function.
Aside from the gas inefficiencies of sequential deposits, it is possible that, because of the swap fees incurred when swapping the provided token for all the others in the Balancer pool, considerably fewer BPTs will be returned than the allowed maxDepositSlippage and maxWithdrawSlippage, potentially DOSing single-token deposits and withdrawals. Hence, the maxDepositSlippage and maxWithdrawSlippage variables might need to be different, depending on whether the called functions deal with multiple pool assets at once.
Note that the above problem also applies when exiting a pool during withdrawals.
Consider always depositing/withdrawing all tokens simultaneously to avoid any reverts and inefficiencies as well as to comply with the best practices defined by the Balancer protocol.
Update: Acknowledged, will resolve. The Origin team stated:
We are aware of this shortcoming and have decided not to fix this issue. Thanks for being thorough and identifying it. Deposits are fine - we have only been planning on using
depositAll, which does support multiple coins at once. We have disabled the single asset deposits for this strategy as they will never be used byVaultCore’sallocatefunction (we never plan on enabling this strategy as an asset default strategy). We plan on adding support for multi-asset withdrawals for all strategies in the future and Balancer’s strategy will get the upgrade at that time, and with that, also gain the benefits of lower gas costs and better capital efficiency.MaxWithdrawal/DepositSlippageis now a lot more lenient (default setting of 1% instead of 0.1%). As per the solution in H-03, theVaultValuechecker will be catching any irregularities (MEV attack attempts).
In the BalancerMetaPoolStrategy contract, the value of 1e50 is used for setting approvals on both stETH and frxETH. There is no clear motivation as to why this number is selected, and the contract lacks inline documentation to clarify its significance.
Consider defining and documenting a constant variable to represent this number, as it is used in multiple places. Following Solidity's style guide, constants should be named in UPPER_CASE_WITH_UNDERSCORES format.
Update: Resolved in pull request #1782.
Throughout the codebase, there are multiple variables named similarly but with vastly different meanings, which makes it difficult for the reader to understand the context. A non-comprehensive list of these is:
_assets and _amounts could be changed to _strategyAssets and _strategyAssetsAmounts.mappedAmounts and mappedAssets could be changed to strategyAssetsToPoolAssets and strategyAssetsAmountsToPoolAssetsAmounts. mappedAssets is especially confusing, as it is very similar to the strategy's assetsMapped, which has a completely different function.asset and amount could be changed to strategyAsset and strategyAssetAmount.wrappedAssetAmounts could be changed to strategyAssetsAmountsToPoolAssetsAmounts.vaultAsset could be changed to strategyAsset.asset could be changed to strategyAsset, or something consistent with the naming previously used. assetAmount could be changed to poolAssetAmountToStrategyAssetAmount.poolAmountsOut could be changed to poolAssetsAmountsOut.In order to improve readability and better convey the code's intentions to the reader, consider modifying variable names to be more aligned with the codebase, while using these suggestions as potential guidelines.
Update: Resolved in pull request #1783.
The checkBalance function is only used in tests.
Consider removing it from the production code in order to improve readability.
Update: Acknowledged, will resolve. The Origin team stated:
We plan on using this read-only function in the future since it will be a much more gas-efficient way of calculating the vault's
totalValue.
In the _deposit function, tokens are checked to ensure they are supported before being deposited into Balancer pools. However, the _withdraw function lacks this check. If the assets supported in the strategy are a subset of the tokens in the Balancer pool, it can result in withdrawing an asset that is not supported by the strategy.
Consider only allowing callers to withdraw assets that are supported by the strategy.
Update: Resolved in pull request #1784.
The IERC20Details and IRETH files are unused. Consider removing them to maintain a cleaner codebase.
Update: Resolved in pull request #1785.
There are several places across the codebase where changes can be made to improve gas consumption.
amount = 0; assignment is unnecessary.fromPoolAsset function that can be used when the amount is not needed. Consider switching to it instead.assetAmount variable is unused, consider removing it.withdraw function, the conversion to a pool asset depends only on poolAssets[i]. Consider doing it only once per pool asset, outside of the for loop that goes through the strategy assets. asset variable can be reused here, to avoid another array access through _assets[i].calldata, consider changing them for future gas optimizations: deposit, _deposit, withdraw, _withdraw, and getBPTExpected.for loop instead.When performing these changes, aim to reach an optimal trade-off between gas optimization and readability. Having a codebase that is easy to understand reduces the chance of errors in the future and improves transparency for the community.
Update: Resolved in pull request #1786.
The following instances of TODO comments were found in the codebase:
TODO comment on line 296 in BalancerMetaPoolStrategy.sol TODO comment on line 380 in BalancerMetaPoolStrategy.solDuring development, having well-described TODO comments will facilitate the process of tracking and solving them. Without this information these comments may age and important information for the security of the system could be forgotten by the time it is released to production.
Consider removing all instances of TODO comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO to the corresponding issues backlog entry.
Update: Resolved in pull request #1796.
In the BaseBalancerStrategy some internal functions start with a leading underscore while others do not.
The use of standard conventions can quickly convey information to developers and users alike. Consider standardizing a naming convention for private and internal functions. A leading underscore is recommended, as mentioned in the Solidity documentation.
Update: Partially resolved in pull request #1797. Some internal functions have not been updated with leading underscores.
mappedAssets Is Initialized With Wrong LengthIn the _deposit function, the length of mappedAmounts and mappedAssets is initialized to tokens.length. This is superfluous if only a subset of the Balancer pool's tokens are supported by the strategy.
While this currently does not present a security risk, consider initializing the array to the correct length.
Update: Resolved in pull request #1800.
Consider addressing the following typographical error in the codebase:
sent should be send.Update: Resolved in pull request #1798.
Withdrawing assets from the strategy calculates maxBPTtoWithdraw , namely how many BPTs should be redeemed with Balancer, in order to get the required liquidity back. Based on Oracle price staleness, this number might be slightly higher than the actual required amount. This means that there could be leftover BPTs in the Strategy contract, after most of them have been spent exiting the Balancer pool. This results in sub-optimal yield generation, as these BPTs should be re-deposited into Aura.
Update: Resolved at commit b9dd480 and documented at commit 6ad405c.
checkBalance Assumes Normal Market ConditionsThe checkBalance functions report the total amount of ETH inside the strategy, by multiplying the balance of BPTs held by their price, in terms of the pool's underlying base asset. The functions assume normal market conditions, meaning the underlying base asset is at 1-1 parity with ETH. This is not the case if stETH or frxETH were to depeg from ETH, in which case the checkBalance functions would report an inflated ETH value inside the strategy.
Update: Partially resolved. Upon conducting additional research, the Origin team stated:
The requirements we would like the
checkBalancefunction to meet:- (critical) Flash loan pool manipulation must not change the total value of units (normalized assets) returned by looping through and adding all the supported pool asset balances together.
- (important)
checkBalancemust report accurate amounts proportional to the share of assets held in the underlying Balancer pool.Conclusion: with the researched approaches, we are able to satisfy only one of the above requirements. We can borrow Balancer’s pool math and calculate the precise amount of assets (the strategy is able to withdraw in any market condition). Unfortunately, such an approach is vulnerable to flash loan sandwich attacks that tilt the pool heavily toward one asset. And while the second requirement would be satisfied, the first one would not be.
For this reason, we have decided to implement a solution that satisfies the first requirement and reports the same token balances no matter the share distribution of the assets in the underlying pool. This does not satisfy the second condition, which makes allocations and correctness regarding withdrawable underlying assets incorrect.
Several high and medium-severity issues were identified. Additionally, suggestions have been made to increase the overall readability and cleanliness of the codebase.
We strongly advise implementing a comprehensive test suite around functionality calculating the underlying value of the strategy. Additionally, the provided documentation has been lacking and confusing at times, as it was not up-to-date with the implementation. We encourage employing better practices around documenting the design decisions behind key functionality and keeping such documentation up-to-date.