6.1 Native token deposits are lost if synchronous deposits are available
The requestDeposit(...) function is designed to handle deposits, including those made with native tokens (e.g., ETH). A recent modification causes requestDeposit(...) to fallback to an internal synchronous deposit mechanism, _syncDeposit(...), if synchronous deposits are enabled in the vault.
The vulnerability arises because _syncDeposit(...) is exclusively designed to handle ERC20 token deposits and does not account for msg.value if the initial call to requestDeposit(...) was made with native tokens.
The relevant internal _syncDeposit(...) logic is:
function _syncDeposit(...) internal onlyOpen returns (uint256 shares) {
ERC7540Storage storage $ = _getERC7540Storage();
shares = _convertToShares(assets, Math.Rounding.Floor);
$.totalAssets += assets;
SafeERC20.safeTransferFrom(IERC20(asset()), owner, safe(), assets);
_mint(receiver, shares);
emit DepositSync(msg.sender, receiver, assets, shares);
}
If a user tries to execute a native token deposit through requestDeposit(...) and synchronous deposits are available the native tokens sent wont be taken into account through the deposit operation.
Recommendation
Consider modifying _syncDeposit(...) to manage native token deposits.
Resolution
We added the native token deposit to the syncDeposit(...) function.
Update from Nethermind Security
The issue is fixed in commit 85ab53.
6.2 NAV proposals may disregard increases to totalAssets from synchronous deposits
The totalAssets variable within the ERC7540 contract, which is critical for share price calculation, is updated through two distinct mechanisms:
- NAV Settlements: During a settlement, the
totalAssets variable is set to the newTotalAssets value proposed by the NAV committee based on their off-chain valuation; - Synchronous Deposits: The introduction of synchronous deposits allows
totalAssets to be incremented directly upon a successful deposit, reflecting the immediate inflow of new assets;
A race condition vulnerability exists due to the potential timing mismatch between these two update paths. If the NAV committee performs a valuation and submits a newTotalAssets proposal before recent synchronous deposits are made (or before they are aware of them), this proposed value will not reflect the assets added by those deposits.
When a settlement subsequently occurs, the totalAssets variable will be overwritten with the NAV committee's proposed (and now outdated) value. This action effectively negates the totalAssets increments from any synchronous deposits that occurred between the NAV committee's valuation time and the settlement execution. While the shares minted from these synchronous deposits remain valid, the totalAssets backing them is temporarily unrepresented in the contract's state, leading to a deflated share price.
Recommendation
To address this vulnerability, it is crucial to ensure that any proposed new valuation accurately incorporates the impact of all synchronous deposits made up to the valuation point. Two primary approaches can be considered:
- Operational Safeguards: Managers could implement a procedural control by delaying the computation of a new NAV until the period for synchronous deposits relevant to that valuation cycle has definitively closed. This would ensure the valuation is based on the most current asset totals. However, it's important to note that such operational measures cannot be enforced at the smart contract level with the existing implementation and rely on consistent off-chain adherence.
- Contract-Level Modification (Delta-Based Updates): A more robust solution involves modifying how
totalAssets is updated during NAV settlements. Instead of overwriting totalAssets with an absolute value proposed by the NAV committee, the system could be designed to apply a delta (i.e., the change in value of the managed assets, excluding synchronous deposits already accounted for on-chain). This approach would ensure that increases to totalAssets from synchronous deposits are preserved, as the NAV committee's reported change would be added to the already updated totalAssets figure, thus preventing the inadvertent exclusion of these deposited assets.
Resolution
In order to solve this issue, we opted for a solution close to the operationnal safeguards. We offer the vault.safe() role (curator) the capacity to expire totalAssets. By doing so he will disable the capacity to do synchronous deposit until next complete totalAssets update, offering the ValuationManager and himself the liberty to update the valuation without risks.
Update from Nethermind Security
The issue is mitigated at commit ca04ed.
6.3 Share price can be artificially inflated through synchronous deposits coupled with share burning
The vault's share price is implicitly determined by the ratio totalAssets / totalSupply. TThe interaction between the two recently introduced features allow users to inflate the share's price:
- Synchronous Deposits: Whitelisted users can directly deposit assets into the vault. This action increases the
totalAssets value and mints a corresponding number of shares to the depositor; - Share Burning: The
burn(...) function allows users to destroy a specified amount of their own shares, thereby decreasing totalSupply;
A malicious (whitelisted) user can exploit this by performing the following sequence:
- Deposit Assets: The attacker deposits a quantity of assets (
X) into the vault. This increases totalAssets by X and mints Y shares for the attacker; - Burn Shares: The attacker then immediately calls the
burn(...) function to destroy all or a significant portion of the Y shares they just received;
The consequence is that the X assets contributed by the attacker remain within the vault (increasing totalAssets, but the corresponding shares that claim those assets are removed from circulation (decreasing totalSupply). This effectively acts as a "donation" of the attacker's deposited assets to the remaining pool, concentrating the vault's total asset value over fewer shares and thereby inflating the price of each remaining share.
Artificially inflating the share price can lead to several adverse consequences. Malicious actors could exploit this vulnerability, particularly early in the vault's operation or when total share supply is low, to conduct "first deposit attacks"; this would cause subsequent legitimate depositors to receive disproportionately fewer shares for their assets, unfairly benefiting the attacker. Furthermore, if the vault's share price is utilized as an oracle by other DeFi protocols for functions like collateral valuation, this manipulation could be leveraged to deceive or exploit those external systems for an attacker's gain. Such potential for manipulation can also undermine user confidence and deter legitimate participation in the vault.
Recommendation
Reconsider the addition of the shares burning feature. The removal of this feature would remove the current possibility of donations through synchronous deposits and burn.
Resolution
We removed the capacity to burn shares.
Update from Nethermind Security
Fixed in commit d05006.
6.4 The requestDeposit(...) function is inconsistent depending on the executed branch
The requestDeposit(...) function has been modified to serve a dual purpose: it initiates an asynchronous deposit request or, if synchronous deposits are currently available, it falls back to executing a synchronous deposit directly. This conditional branching within a single function leads to several inconsistencies in its behavior and outputs, making it more difficult for developers and integrating systems to reliably predict and handle its execution.
The observed inconsistencies include:
- Return Value Discrepancy:
- Asynchronous Path: When initiating an asynchronous deposit,
requestDeposit(...) returns the unique request ID associated with the pending operation. - Synchronous Path: If the function executes a synchronous deposit, it returns 0 (zero), as no asynchronous request ID is generated. Callers must be prepared to handle these two distinct return value meanings.
- Referral Processing:
- Asynchronous Path: Referral parameters are processed and accounted for.
- Synchronous Path: Referral parameters are ignored, and no referral logic is triggered.
- Handling of Existing Pending Requests:
- Asynchronous Path: The function is expected to fail if the caller already has a pending asynchronous deposit request.
- Synchronous Path: If a synchronous deposit is possible, the function may proceed with the synchronous deposit, potentially bypassing or altering the logic that checks for and handles existing pending asynchronous requests for the user. This means the pre-condition regarding pending requests is not consistently enforced.
Recommendation
Consider maintaining requestDeposit(...) exclusively for asynchronous deposit requests and syncDeposit(...) for synchronous deposits.
Resolution
We gave up the "overload" of the requests functions. They will remain as before, and synchronous deposits are only possible by calling the function syncDeposit(...).
Update from Nethermind Security
Fixed in commit d05006.
6.5 syncDeposit(...) function lacks operator and referral features present in asynchronous deposits
The recently introduced syncDeposit(...) function enables users to perform synchronous deposits when this operational mode is active. This provides an immediate deposit pathway, contrasting with the existing asynchronous requestDeposit(...) function.
However, syncDeposit() currently does not implement two features available in the asynchronous deposit flow:
- Operator Feature: The asynchronous
requestDeposit(...) allows for an owner parameter, distinct from msg.sender. This enables an approved operator ( msg.sender) to initiate a deposit request on behalf of the owner of the assets. The syncDeposit(...) function lacks an owner parameter; assets are taken directly from msg.sender, meaning msg.sender is always the owner of the deposited assets. Thus, the pattern of an operator depositing assets on behalf of another distinct owner is not supported; - Referral Feature: The
requestDeposit(...) function includes a referral parameter, allowing for on-chain tracking and potential incentivization of referrals. This parameter is absent in the syncDeposit(...) function signature and its associated logic;
This difference means that users and integrated systems cannot utilize operator-based deposits or referral tracking when using the synchronous deposit method.
- Native Token Deposits: The asynchronous deposit mechanism may support deposits of the chain's native token (e.g., ETH). The
syncDeposit(...) function, as currently implemented, appears to be primarily designed for ERC20 token deposits and lacks a clear mechanism or explicit handling for native token deposits;
Recommendation
To promote consistency and broader utility, consider aligning the feature set of syncDeposit(...) more closely with requestDeposit(...).
Resolution
We added the referral system to the syncDeposit(...) function and use 0 as the requestId. We decided not to support the "operator" feature in synchronous deposits.
Update from Nethermind Security
The Lagoon team has added the desired features. The issue is fixed at commit ca04ed.
