Organization
- @kilnfi
Engagement Type
Spearbit Web3
Period
-
Repositories
Researchers
Findings
Medium Risk
2 findings
2 fixed
0 acknowledged
Low Risk
12 findings
10 fixed
2 acknowledged
Informational
44 findings
36 fixed
8 acknowledged
Medium Risk2 findings
AaveV3Vehicle._maxDeposit implementation logic contains multiple errors
State
Severity
- Severity: Medium
Submitted by
StErMi
Description
The current implementation of the
AaveV3Vehicle._maxDepositfunction contains multiple logic and interpretation errors that incorrectly estimate the max deposit value that the vehicle can supply into the underlying Aave V3 protocol._accruedToTreasuryScaledcould be outdatedThe logic is assuming that the
_accruedToTreasuryScaledvalue returned bypoolDataProvider.getReserveData(...)is always up to date. IflastUpdateTimestamp(always returned by the same function) is lower thanblock.timestampit means that it's possible that more shares could be accounted to the Aave treasury (if there's interest to be accrued). If that's the case (which is high likely) it means that_accruedToTreasuryScaledis outdated and so it could end up returning an overestimatedmaxDepositvalue that could make the deposit operation of the vehicle revertWrong assumptions on the value types
_accruedToTreasuryScaledand_scaledTotalSupplyare values expressed in "share" terms (in the Aave context, "scaled" can be compared to "shares"). Those values must be multiplied by the_liquidityIndexto bring them back to the "non-scaled" value in which_supplyCapis expressed with.Recommendation
Kiln should consider contacting the BGD team to properly integrate the Aave platform and extract the correct logic to determine the max available amount of supply that can be deposited into the Aave v3 Pool for the specific asset.
Kiln
Fixed in commit 53b6e6d
Spearbit
Verified fixes.
CompoundV3Vehicle._maxRedeem Can Misestimate Available Liquidity and Overstate Redeemable Shares
State
Severity
- Severity: Medium
Submitted by
Alireza Arjmand
Description
CompoundV3Vehicle._maxRedeemderives redeemable liquidity from:$.compoundV3Market.totalSupply() - $.compoundV3Market.totalBorrow()and then converts that amount into shares. This assumption is not always valid in Compound V3 and can cause the vehicle to overestimate how much can actually be redeemed.
Two cases make this especially problematic:
-
Borrows can exceed supply net of reserves: In Compound V3, borrows can effectively eat into reserves, so
totalBorrow()is not guaranteed to stay belowtotalSupply(). As a result,totalSupply() - totalBorrow()can underflow or otherwise stop being a reliable proxy for withdrawable base liquidity. -
Absorbs can inflate
totalSupply()without fresh incoming base assets: Duringabsorb, Compound updates accounting by increasing total supply and decreasing total borrow as debt is socialized onto reserves, but this does not mean new base tokens were actually transferred into the protocol. If reserves were previously withdrawn, an absorb can maketotalSupply() - totalBorrow()look healthier than the protocol’s real withdrawable base balance, causing_maxRedeemto overestimate redeemable shares.
As a result,
maxRedeemmay report more redeemable shares than can actually be withdrawn from Compound V3, leading to incorrect vehicle limits and failed redemption attempts.Proof of Concept
The proof of concept can be seen below:
// SPDX-License-Identifier: BUSL-1.1// SPDX-FileCopyrightText: 2025 Kiln <[email protected]>//// d8b 888 888// Y8P 888 888// 888 888// 888d888 8888b. 888 888 88888b. .d88b. 888888// 888P" "88b 888 888 888 "88b d8P Y8b 888// 888 .d888888 888 888 888 888 88888888 888// 888 888 888 888 888 888 888 Y8b. Y88b.// 888 "Y888888 888 888 888 888 "Y8888 "Y888//pragma solidity >=0.8.33; import {Test, console2} from "forge-std/Test.sol"; import {ICreateX} from "@createx/ICreateX.sol";import {IERC20} from "@openzeppelin-contracts/token/ERC20/IERC20.sol"; import {Asset, EstimationType, Query, Mode, State} from "src/steam/Query.sol"; import {ExternalAccessControl, IExternalAccessControl} from "src/common/ExternalAccessControl.sol";import {FreezablePausableBeacon} from "src/common/FreezablePausableBeacon.sol";import {CoreFactory} from "src/factories/CoreFactory.sol";import {CompoundV3VehicleFactory} from "src/factories/vehicles/CompoundV3VehicleFactory.sol";import {FeeManagerFactory} from "src/factories/vehicles/FeeManagerFactory.sol";import {Roles} from "src/libs/Roles.sol";import {FeeManager} from "src/vehicles/base/FeeManager.sol";import {IFeeManager} from "src/vehicles/base/interfaces/IFeeManager.sol";import {ModulesManager} from "src/vehicles/base/ModulesManager.sol";import {CompoundV3Vehicle} from "src/vehicles/compound_v3/CompoundV3Vehicle.sol";import {IComet} from "src/vehicles/compound_v3/interfaces/IComet.sol"; contract CompoundV3VehicleMinimalForkTest is Test { address internal constant MAINNET_CREATEX = 0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed; address internal constant COMET_USDC_V3 = 0xc3d688B66703497DAA19211EEdff47f25384cdc3; address internal constant USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48; address internal constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; uint256 internal constant BLOCK_BEFORE_24524672 = 24_524_671; address internal constant ABSORB_ABSORBER = 0x6905Cca5716Cc4FAe5692647F3b09A54F2754a87; address internal constant ABSORBED_ACCOUNT = 0x26922eFd8491b61Ea21F101D075D4abd1d8a953e; uint256 internal constant BOB_WETH_COLLATERAL = 200_000 ether; uint256 internal constant INITIAL_DEPOSIT = 100e6; uint256 internal constant ALICE_DEPOSIT = 1_000e6; uint256 internal constant BOB_DEPOSIT = 1e6; uint256 internal constant DUST_DEPOSIT = 1; address internal ADMIN; address internal DEPLOYER; address internal ALICE; address internal BOB; address internal TREASURY; CompoundV3Vehicle internal vehicle; CoreFactory internal coreFactory; ExternalAccessControl internal accessControl; CompoundV3VehicleFactory internal compoundV3Factory; function setUp() external { vm.createSelectFork(vm.envString("MAINNET_RPC")); ADMIN = makeAddr("Admin"); DEPLOYER = makeAddr("Deployer"); ALICE = makeAddr("Alice"); BOB = makeAddr("Bob"); TREASURY = makeAddr("Treasury"); _deployVehicleOnCurrentFork(); } function _deployVehicleOnCurrentFork() internal { coreFactory = new CoreFactory(ICreateX(MAINNET_CREATEX)); IExternalAccessControl.RoleAttribution[] memory initialRoles = new IExternalAccessControl.RoleAttribution[](1); initialRoles[0] = IExternalAccessControl.RoleAttribution({role: Roles.FACTORY_SPAWN, account: DEPLOYER}); accessControl = new ExternalAccessControl(1 days, ADMIN, initialRoles); CompoundV3Vehicle implementation = new CompoundV3Vehicle(); FreezablePausableBeacon beacon = new FreezablePausableBeacon(address(implementation), accessControl); compoundV3Factory = new CompoundV3VehicleFactory(coreFactory, accessControl, beacon, 0); deal(USDC, DEPLOYER, 10_000e6); vm.startPrank(DEPLOYER); IERC20(USDC).approve(address(compoundV3Factory), type(uint256).max); vehicle = compoundV3Factory.spawn( CompoundV3VehicleFactory.SpawnParams({ compoundV3Market: COMET_USDC_V3, accessControl: ExternalAccessControl(address(0)), feeManager: FeeManager(address(0)), modulesManager: ModulesManager(address(0)), querySalt: keccak256("bootstrap-query"), deploymentSalt: keccak256("bootstrap-deployment"), initialDepositSize: INITIAL_DEPOSIT, initialExpectedSupply: 1 }) ); vm.stopPrank(); assertEq(vehicle.asset(), USDC); assertEq(IComet(COMET_USDC_V3).baseToken(), USDC); assertTrue(vehicle.isEnabled()); } function test_absorb_can_make_max_redeem_optimistic1() external { // "Start of block 24524672" corresponds to state at the end of block 24524671. vm.createSelectFork(vm.envString("MAINNET_RPC"), BLOCK_BEFORE_24524672); _deployVehicleOnCurrentFork(); deal(USDC, ALICE, ALICE_DEPOSIT); uint256 aliceShares = _depositAndUnlock(vehicle, ALICE, ALICE_DEPOSIT, keccak256("alice-absorb-deposit")); assertGt(aliceShares, 0); uint256 bobBorrowed = _borrowAvailableLiquidityAsBob(); assertGt(bobBorrowed, 0); uint256 cometUsdcCashBeforeAbsorb = IERC20(USDC).balanceOf(COMET_USDC_V3); uint256 reportedLiquidityBeforeAbsorb = _reportedCometAvailableLiquidity(); address[] memory absorbAccounts = new address[](1); absorbAccounts[0] = ABSORBED_ACCOUNT; bytes memory cometTotalSupply; bytes memory cometTotalBorrow; (, cometTotalSupply) = COMET_USDC_V3.call(abi.encodeWithSignature("totalSupply()")); (, cometTotalBorrow) = COMET_USDC_V3.call(abi.encodeWithSignature("totalBorrow()")); console2.log("cometTotalSupply ", uint256(bytes32(cometTotalSupply))); console2.log("cometTotalBorrow ", uint256(bytes32(cometTotalBorrow))); // MethodID 0xc3cecfd2: absorb(address,address[]) // (bool absorbSuccess,) = // COMET_USDC_V3.call(abi.encodeWithSelector(bytes4(0xc3cecfd2), ABSORB_ABSORBER, absorbAccounts)); // assertTrue(absorbSuccess); // (, cometTotalSupply) = COMET_USDC_V3.call(abi.encodeWithSignature("totalSupply()")); // (, cometTotalBorrow) = COMET_USDC_V3.call(abi.encodeWithSignature("totalBorrow()")); // console2.log("cometTotalSupply ", uint256(bytes32(cometTotalSupply))); // console2.log("cometTotalBorrow ", uint256(bytes32(cometTotalBorrow))); // Asset[] memory maxRedeemAssets = vehicle.maxRedeem(ALICE); --> fails Query memory redeemQuery = Query({ owner: ALICE, receiver: ALICE, input: _singleAsset(address(vehicle), 1), output: new Asset[](0), mode: Mode.REDEEM, salt: keccak256("alice-max-redeem-after-absorb"), data: "" }); vm.startPrank(ALICE); IERC20(address(vehicle)).approve(address(vehicle), 1); vm.expectRevert(); vehicle.create(redeemQuery); // FAIL: panic: arithmetic underflow or overflow (0x11) // cometTotalSupply 381832487932364 // cometTotalBorrow 381832487932375 vm.stopPrank(); } function _borrowAvailableLiquidityAsBob() internal returns (uint256 borrowedAssets) { deal(WETH, BOB, BOB_WETH_COLLATERAL); vm.startPrank(BOB); IERC20(WETH).approve(COMET_USDC_V3, BOB_WETH_COLLATERAL); IComet(COMET_USDC_V3).supply(WETH, BOB_WETH_COLLATERAL); vm.stopPrank(); uint256 reportedLiquidity = _reportedCometAvailableLiquidity(); assertGt(reportedLiquidity, 1); // Target near-full utilization. If account constraints prevent full drain, find Bob's max. uint256 borrowTarget = reportedLiquidity + 10; // ---> Here we want to exceed the supply rate borrowedAssets = _findMaxBorrowableAsBob(borrowTarget); assertGt(borrowedAssets, 0); bytes memory cometTotalSupply; bytes memory cometTotalBorrow; (, cometTotalSupply) = COMET_USDC_V3.call(abi.encodeWithSignature("totalSupply()")); (, cometTotalBorrow) = COMET_USDC_V3.call(abi.encodeWithSignature("totalBorrow()")); console2.log("cometTotalSupply ", uint256(bytes32(cometTotalSupply))); console2.log("cometTotalBorrow ", uint256(bytes32(cometTotalBorrow))); vm.prank(BOB); IComet(COMET_USDC_V3).withdraw(USDC, borrowedAssets); (, cometTotalSupply) = COMET_USDC_V3.call(abi.encodeWithSignature("totalSupply()")); (, cometTotalBorrow) = COMET_USDC_V3.call(abi.encodeWithSignature("totalBorrow()")); console2.log("cometTotalSupply ", uint256(bytes32(cometTotalSupply))); console2.log("cometTotalBorrow ", uint256(bytes32(cometTotalBorrow))); } function _findMaxBorrowableAsBob(uint256 upperBound) internal returns (uint256 maxBorrow) { uint256 low = 0; uint256 high = upperBound; while (low < high) { uint256 mid = low + (high - low + 1) / 2; uint256 snap = vm.snapshotState(); vm.prank(BOB); (bool success,) = COMET_USDC_V3.call(abi.encodeWithSelector(IComet.withdraw.selector, USDC, mid)); vm.revertToState(snap); if (success) { low = mid; } else { high = mid - 1; } } return low; } function _reportedCometAvailableLiquidity() internal view returns (uint256) { uint256 totalSupply_ = IComet(COMET_USDC_V3).totalSupply(); uint256 totalBorrow_ = IComet(COMET_USDC_V3).totalBorrow(); return totalBorrow_ >= totalSupply_ ? 0 : totalSupply_ - totalBorrow_; } function _singleAsset(address asset, uint256 value) internal pure returns (Asset[] memory assets) { assets = new Asset[](1); assets[0] = Asset({asset: asset, value: value}); } function _depositAndUnlock(CompoundV3Vehicle target, address owner, uint256 assets, bytes32 salt) internal returns (uint256 sharesReceived) { Asset[] memory input = _singleAsset(USDC, assets); Query memory query = Query({ owner: owner, receiver: owner, input: input, output: target.estimate(input, Mode.DEPOSIT, EstimationType.OUTPUT), mode: Mode.DEPOSIT, salt: salt, data: "" }); vm.startPrank(owner); IERC20(USDC).approve(address(target), assets); State createState = target.create(query); assertEq(uint256(createState), uint256(State.UNLOCKING)); (State unlockState, Asset[] memory unlockedAssets) = target.unlock(query); vm.stopPrank(); assertEq(uint256(unlockState), uint256(State.SETTLED)); assertEq(unlockedAssets.length, 1); assertEq(unlockedAssets[0].asset, address(target)); sharesReceived = unlockedAssets[0].value; }}Recommendation
Do not use
totalSupply() - totalBorrow()as a proxy for immediately withdrawable base liquidity.Comet doesn't cap the amount that can be withdrawn to any address, effectively capping it to the minimum of the user's balance or the protocol's balance.
Kiln
Fixed by https://github.com/kilnfi/railnet/pull/375/changes/c33a3aff23872ebe2d108200f00144010fdfc8cc
Spearbit
Verified fix, the
compoundV3Vehiclenow uses the contract's remaining base asset balance as reference for maximum redeemable amount. Note: the CompoundV3Vehicle has since been fully removed from the codebase in later commits.
Low Risk12 findings
Share decimals and asset decimals are swapped in the fee manager calls
State
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: High Submitted by
zigtur
Description
BaseVehiclecalls bothonOperations()andpreviewOnOperations()as part of the contract.FeeManager.onOperations()is declared as the following.function onOperations( uint256 currentTotalSupply, uint256 currentTotalAssets, uint8 assetDecimals, uint8 sharesDecimals ) external returns (uint256 feeSharesToMint) {However,
BaseVehiclecalls bothonOperations()andpreviewOnOperations()withsharesDecimalsandassetDecimalsin the wrong order.function _handleFeesBeforeOperation(uint256 totalSupply_, uint256 totalAssets_, Id qid) internal returns (uint256) { // ... uint256 _feeSharesToMint = _feeManager.onOperations(totalSupply_, totalAssets_, decimals(), _assetDecimals()); // ... }Proof of Concept
The following test can be imported in
test/vehicles/base/FeeManager.t.sol.// ============================================================================ // Decimals Confusion Tests // ============================================================================ /// @notice Demonstrates that swapping assetDecimals and sharesDecimals in previewOnOperations /// produces different fee share amounts, exposing the parameter ordering confusion /// between onOperations (sharesDecimals, assetDecimals) and previewOnOperations (assetDecimals, sharesDecimals). function testPreviewOnOperationsDecimalsConfusion() public { // Setup: 20% performance fee IFeeManager.Fees memory _fees = IFeeManager.Fees({ performanceFeeBps: 2000, managementFeeBps: 200, // 2% annually depositFeeBps: 0, redeemFeeBps: 0 }); IFeeManager.Fees memory _maxFees = IFeeManager.Fees({ performanceFeeBps: 5000, managementFeeBps: 1000, depositFeeBps: 1000, redeemFeeBps: 1000 }); IFeeManager.FeeRecipient[] memory _recipients = _createDefaultRecipients(); $feeManager = _deployFeeManager(_fees, _maxFees, _recipients); uint256 _initialAssets = 10e6; // 6-decimal asset (e.g. USDC) uint256 _currentAssets = 11e6; // 100e6 profit uint256 _totalSupply = 0; // 18-decimal shares // Set initial cache vm.prank($mockVehicle); $feeManager.onUpdate(_initialAssets); // Fast forward 1 year (so management fee also kicks in) vm.warp(block.timestamp + 365 days); // Call 1: Correct ordering — assetDecimals=6, sharesDecimals=18 vm.prank($mockVehicle); uint256 _feeSharesCorrect = $feeManager.previewOnOperations(_totalSupply, _currentAssets, 6, 18); // Call 2: Swapped ordering — assetDecimals=18, sharesDecimals=6 (the confusion) vm.prank($mockVehicle); uint256 _feeSharesSwapped = $feeManager.previewOnOperations(_totalSupply, _currentAssets, 18, 6); emit log_named_uint("Fee shares (assetDecimals=6, sharesDecimals=18)", _feeSharesCorrect); emit log_named_uint("Fee shares (assetDecimals=18, sharesDecimals=6)", _feeSharesSwapped); emit log_named_uint("Difference", _feeSharesCorrect > _feeSharesSwapped ? _feeSharesCorrect - _feeSharesSwapped : _feeSharesSwapped - _feeSharesCorrect); // These should differ if the decimals actually matter for the conversion // This demonstrates the impact of the confusion if (_feeSharesCorrect != _feeSharesSwapped) { emit log("DECIMALS CONFUSION: different results when swapping assetDecimals and sharesDecimals"); } else { emit log("No difference detected (same decimals or scaling cancels out)"); } }Recommendation
The
FeeManager.onOperationsdeclaration must be fixed such that the ordering is correct.The following patch fixes the external functions declarations.
Please note that internal functions are still using the order pattern that led to the issue (i.e.
shareSupply, assetSupply, assetDecimals, shareDecimals). This is error-prone.diff --git a/src/vehicles/base/FeeManager.sol b/src/vehicles/base/FeeManager.solindex 3042b703..86420ead 100644--- a/src/vehicles/base/FeeManager.sol+++ b/src/vehicles/base/FeeManager.sol@@ -273,8 +273,8 @@ contract FeeManager is IFeeManager, Interceptor, ReentrancyGuardUpgradeable, Ext function onOperations( uint256 currentTotalSupply, uint256 currentTotalAssets,- uint8 assetDecimals,- uint8 sharesDecimals+ uint8 sharesDecimals,+ uint8 assetDecimals ) external returns (uint256 feeSharesToMint) { (uint256 _managementFeeSharesToMint, uint256 _performanceFeeSharesToMint) = _previewOnOperations(currentTotalSupply, currentTotalAssets, assetDecimals, sharesDecimals);@@ -302,8 +302,8 @@ contract FeeManager is IFeeManager, Interceptor, ReentrancyGuardUpgradeable, Ext function previewOnOperations( uint256 currentTotalSupply, uint256 currentTotalAssets,- uint8 assetDecimals,- uint8 sharesDecimals+ uint8 sharesDecimals,+ uint8 assetDecimals ) external view returns (uint256 feeSharesToMint) { (uint256 _managementFeeSharesToMint, uint256 _performanceFeeSharesToMint) = _previewOnOperations(currentTotalSupply, currentTotalAssets, assetDecimals, sharesDecimals);Kiln
Fixed in commit 35678cd and commit 644c809.
Spearbit
Fixed. The "on operations" functions are now the correct ordering for input parameters.
Shares and assets downscaling always round down instead of using the rounding input parameter
State
Severity
- Severity: Low
Submitted by
zigtur
Description
The
convertToAssetsandconvertToSharesfunctions in theShares.solfile implement an upscaling for calculations and then a downscaling of the result. These functions provide aroundinginput parameter to select which rounding direction to use:FloororCeil.However when
rounding = Ceil, only theMath.mulDivis rounding up and not thedownscaleShares/downscaleAssetsoperation. This leads to rounding down when it should round up.In the current state of the codebase, the issue occurs only on estimation calculations.
Proof of Concept
Import the following as
test/vehicles/erc4626/SharesCeilRoundingBug.t.soland then execute the following commands.forge test --match-contract "ERC4626VehicleCeilRoundingBugTest" --mt test_vehicle_estimateDepositInput_CeilRoundingBug -vv// SPDX-License-Identifier: BUSL-1.1pragma solidity >=0.8.33; import {Test} from "@forge-std/Test.sol";import {Math} from "@openzeppelin-contracts/utils/math/Math.sol";import {SharesLib} from "src/libs/Shares.sol";import {Query, Route, Mode, Asset, State, EstimationType} from "src/steam/Query.sol";import {ERC4626VehicleFactory} from "src/factories/vehicles/ERC4626VehicleFactory.sol";import {ERC4626Mock} from "test/vehicles/mocks/ERC4626Mock.sol";import {ERC20Mock} from "test/vehicles/mocks/ERC20Mock.sol";import {STEAMSingleAssetTester} from "test/test_utils/STEAM.SingleAsset.t.sol";import {Tester, TesterCommon, TesterTypes} from "test/test_utils/steam/utils/Tester.sol";import {ExternalAccessControl} from "src/common/ExternalAccessControl.sol";import {FeeManager} from "src/vehicles/base/FeeManager.sol";import {ModulesManager} from "src/vehicles/base/ModulesManager.sol";import {BaseVehicle} from "src/vehicles/base/BaseVehicle.sol";import {IERC20Metadata} from "@openzeppelin-contracts/token/ERC20/extensions/IERC20Metadata.sol"; /// @title PoC: SharesLib.convertToAssets Ceil rounding defeated by downscale truncation/// @notice When sharesDecimals(18) > assetDecimals(6), the floor division in/// downscaleAssets() negates the Ceil rounding from Math.mulDiv()./// This violates the invariant: convertToAssets(..., Ceil) >= convertToAssets(..., Floor)/// for the same inputs when the mathematical result is non-integer.////// Affected production paths (all with USDC/USDT/WBTC underlying):/// - SingleAssetBaseVehicle._convertToAssets (estimate DEPOSIT+INPUT)/// - MultiVehicle._convertToAssets (estimate DEPOSIT+INPUT)/// - Conduit._convertConduitSharesToVehicleShares (estimate INPUT) // =============================================================================// PART 1: ERC4626 Vehicle end-to-end test// ============================================================================= contract ERC4626VehicleCeilRoundingBugTest is STEAMSingleAssetTester { using Tester for Query; using TesterCommon for TesterTypes.AssertBalance[]; using TesterCommon for TesterTypes.AssertAmount[]; ERC4626Mock internal $underlyingAsset; function setUp() public virtual { // Deploy USDC-like token (6 decimals) and ERC4626 vault $asset = new ERC20Mock("USDC", "USDC", 6); $underlyingAsset = new ERC4626Mock(address($asset), "Vault", "vUSDC", 12); // Deploy vehicle through factory (initial deposit = 1 USDC = 1e6) ($vehicle,) = _deployERC4626Vehicle( _deployCoreFactory(), ERC4626VehicleFactory.SpawnParams({ vault: address($underlyingAsset), accessControl: ExternalAccessControl(address(0)), feeManager: FeeManager(address(0)), modulesManager: ModulesManager(address(0)), querySalt: keccak256("INITIAL_DEPOSIT_QUERY"), deploymentSalt: keccak256("DEPLOYMENT_SALT"), initialDepositSize: 1e6, // 1 USDC initialExpectedSupply: 1e18 }) ); vm.label(address($asset), "USDC"); vm.label(address($underlyingAsset), "vUSDC-vault"); vm.label(address($vehicle), "vehicle"); } /// @notice End-to-end PoC through the vehicle's estimate() function. /// /// Setup: /// 1. After setUp: totalSupply=1e18, totalAssets=1e6 (1:1 rate) /// 2. Alice deposits 2 USDC → totalSupply=3e18, totalAssets=3e6 /// 3. Simulate loss: set vault USDC to 2e6 → totalSupply=3e18, totalAssets=2e6 /// /// Bug demonstration: /// estimate([{vehicle, 1e18}], DEPOSIT, INPUT) asks: /// "How many USDC do I need to deposit to get 1e18 shares?" /// Uses Ceil rounding (favor protocol: charge user more). /// math: 1e18 * 2e6 / 3e18 = 666_666.67 → Ceil = 666_667 USDC-wei /// But returns: 666_666 (same as Floor, user is undercharged by 1 USDC-wei) function test_vehicle_estimateDepositInput_CeilRoundingBug() public { address alice = makeAddr("alice"); // Step 1: Verify initial state assertEq($vehicle.totalSupply(), 1e18, "Initial totalSupply"); assertEq($vehicle.totalAssets(), 1e6, "Initial totalAssets"); assertEq($vehicle.decimals(), 18, "Vehicle shares = 18 decimals"); assertEq($asset.decimals(), 6, "USDC = 6 decimals"); // Step 2: Alice deposits 2 USDC to get totalSupply=3e18 uint256 depositAmount = 2e6; Query memory _depositQuery = _get_and_prepare_query(alice, alice, Mode.DEPOSIT, depositAmount); vm.prank(alice); $vehicle.create(_depositQuery); vm.prank(alice); $vehicle.unlock(_depositQuery); assertEq($vehicle.totalSupply(), 3e18, "totalSupply after deposit"); assertEq($vehicle.totalAssets(), 3e6, "totalAssets after deposit"); // Step 3: Simulate a loss — reduce vault's USDC balance to 2e6 // This creates totalSupply=3e18, totalAssets=2e6 (non-trivial exchange rate) deal(address($asset), address($underlyingAsset), 2e6); assertEq($vehicle.totalAssets(), 2e6, "totalAssets after simulated loss"); assertEq($vehicle.totalSupply(), 3e18, "totalSupply unchanged"); // Step 4: Estimate DEPOSIT+INPUT — "How many USDC for 1e18 shares?" // This path uses Ceil rounding (SingleAssetBaseVehicle._convertToAssets with roundUp=true) Asset[] memory depositSharesOutput = new Asset[](1); depositSharesOutput[0] = Asset({asset: address($vehicle), value: 1e18}); Asset[] memory assetsNeeded = $vehicle.estimate(depositSharesOutput, Mode.DEPOSIT, EstimationType.INPUT); // Mathematical result: 1e18 * 2e6 / 3e18 = 666_666.666... USDC-wei // With Ceil: should be 666_667 (round up to charge user more, favor protocol) // BUG: returns 666_666 (same as Floor, protocol is shortchanged) assertEq(assetsNeeded[0].asset, address($asset), "Output asset is USDC"); assertEq(assetsNeeded[0].value, 666_666, "BUG: estimate returns 666_666 (should be 666_667)"); // Verify Floor also returns 666_666 using SharesLib directly uint256 floorResult = SharesLib.convertToAssets( 1e18, 18, 6, $vehicle.totalSupply(), $vehicle.totalAssets(), Math.Rounding.Floor ); assertEq(floorResult, 666_666, "Floor = 666_666"); // Ceil == Floor, violating the rounding invariant assertEq(assetsNeeded[0].value, floorResult, "BUG: Ceil estimate == Floor"); // Step 5: Actually deposit the estimated amount and verify shares received // The estimate said 666_666 USDC buys 1e18 shares — let's test that claim. address bob = makeAddr("bob"); uint256 estimatedCost = assetsNeeded[0].value; // 666_666 uint256 bobSharesBefore = $vehicle.balanceOf(bob); assertEq(bobSharesBefore, 0, "Bob starts with 0 shares"); Query memory _bobDeposit = _get_and_prepare_query(bob, bob, Mode.DEPOSIT, estimatedCost); vm.prank(bob); $vehicle.create(_bobDeposit); vm.prank(bob); $vehicle.unlock(_bobDeposit); uint256 bobSharesAfter = $vehicle.balanceOf(bob); uint256 actualSharesReceived = bobSharesAfter - bobSharesBefore; // Bob deposited exactly what the estimate said was needed for 1e18 shares, // but received fewer: 999_999_000_000_000_000 instead of 1_000_000_000_000_000_000 // The shortfall is 1_000_000_000_000 (1e12) shares. assertEq(actualSharesReceived, 999_999_000_000_000_000, "Actual shares received"); assertTrue(actualSharesReceived < 1e18, "BUG: Got fewer shares than the estimate promised"); assertEq(1e18 - actualSharesReceived, 1e12, "Shortfall = 1e12 shares"); }}Recommendation
The downscaling should round up when
Ceilis used.Note: double rounding up should be avoided.
Kiln
Fixed in commit 10bbcb2.
Spearbit
Fixed. The
roundingparameter is now used.However, there is now a double ceil rounding in the function. In the current state of the codebase, it does not have any impact. It should be documented as it could become and issue in the future.
Scoped-role management functions should follow the same sanity rules defined in AccessControlDefaultAdminRules
State
- Acknowledged
Severity
- Severity: Low
Submitted by
StErMi
Description
The
grantScopedRole,revokeScopedRoleandrenounceScopedRoleare not following the same sanity check rules adopted by theAccessControlDefaultAdminRulescontract whichExternalAccessControlis inheriting from.The "grant" operation, even for "scoped roles", should revert if the
roleis theDEFAULT_ADMIN_ROLEUnlike
ExternalAccessControl.grantRole, which invokesAccessControlDefaultAdminRules.grantRole, the custom one "reimplement" the logic.Note that it will also emit the event
RoleGranted.You should do the following changes to align the behavior to the one expected by
AccessControlDefaultAdminRules:grantScopedRoleshould revert with the errorAccessControlEnforcedDefaultAdminRuleswhenrole == DEFAULT_ADMIN_ROLE.function grantScopedRole(bytes32 role, address scope, address account) public virtual onlyRole(getRoleAdmin(role)) notPublicScopedRole(role, scope){ if (role == DEFAULT_ADMIN_ROLE) { revert AccessControlEnforcedDefaultAdminRules(); } _grantScopedRole(role, scope, account);} function _grantScopedRole(bytes32 role, address scope, address account) internal virtual returns (bool) { bytes32 _encodedRole = _encodeScopedRole(role, scope); bool granted = _grantRole(_encodedRole, account); if( granted ) { emit ScopedRoleGranted(role, scope, account, _msgSender()); } return granted;}The same sanity check should be implemented in the
revokeScopedRoleandrenounceScopedRolefunctions.Recommendation
Kiln should revert the
grantScopedRole,revokeScopedRoleandrenounceScopedRolewhen theroleisDEFAULT_ADMIN_ROLEto follow the same logic and behavior adopted by theAccessControlDefaultAdminRulesfrom which it is inheriting from.Kiln
Acknowledged. Multiple scoped DEFAULT_ADMIN_ROLE holders per scope is intentional behavior. Only the global DEFAULT_ADMIN_ROLE can grant scoped admin roles, scoped admins cannot sub-delegate. This diverges from AccessControlDefaultAdminRules' single-admin constraint by design, as we see valid operational scenarios for multiple scoped admins existing in parallel. Enforcing uniqueness would require a significant refactor to disambiguate global vs encoded scoped roles which we decided against given the trust model and the added complexity it would require. NatSpec has been updated to clearly disclose this behavior.
Spearbit
Acknowledged.
Euler EVault view functions integration safety relies on the governance
State
- Acknowledged
Severity
- Severity: Low
Submitted by
zigtur
Description
_maxRedeemand_maxDepositintegrates with Euler EVault through_underlyingVault.maxWithdraw(...)and_underlyingVault.maxDeposit(...).In Euler docs about hooks, there is the specific mention that:
ERC-4626 Compliance
- Some hook configurations may cause the vault to not be fully ERC-4626 compliant
- The max* functions may be inaccurate with hooks installed
The
maxDepositandmaxRedeemfunctions may not be accurate. However, the vehicle relies on them.This "non-compliance", combined with the fact that hooks can be modified on EVaults that have a governance can lead to breaking the ERC4626 vehicle (denial of service).
Recommendation
The EVault governance actions should be monitored for vehicles integrating with an EVault.
Kiln
Acknowledged.
Spearbit
Acknowledged.
AaveV3Vehicle prevents users from redeeming shares when the Aave reserve is frozen
State
Severity
- Severity: Low
Submitted by
StErMi
Description
When the Aave v3 reserve (for the specific underlying) is frozen, only the Supply and Borrow operations are blocked. The user can always repay or withdraw from that reserve.
The current implementation of
AaveV3Vehicle._maxRedeemis instead settingassets[0].value = 0when the reserve is frozen, not allowing the user to redeem the vehicle's shares when theif (!AssetLib.assetsMatching(query.input, _max)) {check is executed{ Asset[] memory _max = query.mode == Mode.DEPOSIT ? _maxDeposit(msg.sender, __totalSupply, __totalAssets) : _maxRedeem(msg.sender, __totalSupply, __totalAssets); if (!AssetLib.assetsMatching(query.input, _max)) { if (query.mode == Mode.DEPOSIT) { revert ErrorLib.MaxDepositTooLow(query.input, _max); } else { revert ErrorLib.MaxRedeemTooLow(query.input, _max); } }}This wrong assumption will also influence the value returned by the
BaseVehicle.maxRedeemwhen the reserve is frozen and all the integrators and contract that will base their logic on such value.Recommendation
Kiln should remove the
!_isFrozencheck from theif (_isActive && !_isPaused && !_isFrozen) {condition in theAaveV3Vehicle._maxRedeemlogic.The
bool _isFrozenflag can be fully ignored when fetched from the_poolDataProvider.getReserveConfigurationData(address($.asset));response.Kiln
Fixed by commit
cc73b4c40b3cef483ed0554b128ecf45031a23bc.Spearbit
Fixed.
BaseVehicle.ready is not compliant with the STEAM standard
Severity
- Severity: Low
Submitted by
StErMi
Description
The STEAM standard documentation relative to the
ready()function statesIndicates whether the Vehicle is operational and ready to accept queries.
MUST return
trueif the Vehicle is operational and can acceptcreate()calls.The current implementation of the function is always returning
trueeven if the vehicle has not been enabled yet. When the vehicle is not enabled, only the$.deployeraddress will be able to execute thecreatefunction and continue the query's lifecycle.Recommendation
Kiln should update the
readyfunction to returntrueonly when the vehicle has been fully initialized and enabled and everyone can execute thecreatefunction.Kiln should also consider to "merge" the
ready()andisEnabled()functions if they express the same meaning.Kiln
Fixed by commit f3ae2878587c18b4a81ea386c2fa9bc1d6042b4e
Spearbit
Fixed.
BaseVehicle._payout should never be able to revert
State
Severity
- Severity: Low
Submitted by
StErMi
Description
The current implementation of the
_payoutflow in theBaseVehicleis executed when the Query is in theRECOVERINGorUNLOCKINGstate.For the sake of the review we will only describe the
UNLOCKINGscenario given that no vehicle, for this specific review, can enter theRECOVERINGstate.To be in the
UNLOCKINGscenario, the user must have executed thecreateflow and so deposited funds (underlying or vehicle's share) into the vehicle.After executing
unlockthe user would receive back the vehicle's shares (if the query has been created withmode=DEPOSIT) or underlying (if the query has been created withmode=REDEEM).If the
_payoutfunction reverts, the user's funds will be stuck inside the vehicle without a way to recover them.Right now the
_payoutfunction can revert in three casespaidAssets.length != assets.length || (_hasFees && _feeAssets.length != assets.length->revert InvalidPayoutArrayLength_payoutAmount + _feeAmount != assets[_idx].value->revert BaseVehicleErrors.InvalidPayout_payoutAsset != IERC20(assets[_idx].asset) || _feeAsset != _payoutAsset->revert InvalidPayoutAsset
In all of these three cases the only possible reason to revert is that the
FeeManageris not working as expected or is broken.It's fair to assume that the
FeeManageris a fully trusted, vetted and always working core component of the Railnet protocol. Given such assumption all of these three revert checks should be removed.Recommendation
Given the assumption made on the
FeeManager, Kiln should remove the three mentionedrevertcases in theBaseVehicle._payoutKiln
Fixed by commit
d23ce2b1ad361377d9a7daaa4cc591e5dda909e6Spearbit
Fixed.
FeeManager.onUpdate should revert when $.currentFeesConfigId == bytes32(0)
State
Severity
- Severity: Low
Submitted by
StErMi
Description
When
FeeManager.onUpdateis executed and$.currentFeesConfigIdis equal tobytes32(0)it means that the__FeeManager_inithas not been executed yet.When a Vehicle is deployed and initialized, the
onUpdatefunction is always executed. The Vehicle, should not be able to initialize itself if theFeeManagerhas not been also properly initialized.Recommendation
Kiln should revert in the
onUpdateflow if theFeeManagerhas not been properly initialized yet.Kiln
Fixed by commit
e1d58cee3f90fb81f4535e16b2649679d1de3e6b.Spearbit
Fixed. Kiln has decided to avoid reverting directly in the
FeeManager.onUpdateas suggested and has opted to revert duringBaseVehicleInitialization.__BaseVehicle_initwhen the Fee Config ID returned by theoptionalFeeManager_is equal tobytes32(0)Aave offers multiple distribution reward systems which are currently not supported by the Aave V3 Vehicle
State
Severity
- Severity: Low
Submitted by
StErMi
Description
The current Aave V3 vehicle implementation does only support the "Aave Reward Controller" reward system which seems to be going to be deprecated in favor of other systems.
In the specific, by looking at least at their "Core Instance" markets for the Mainnet chain we see that rewards are distributed via:
- Custom reward programs offered "directly" by the underlying protocol
Wrapped eETHhas a "custom" rewards program that is distributed directly by Ether.fi (Loyalty Points). See https://etherfi.gitbook.io/etherfi/getting-started/loyalty-pointsUSDehas a "custom" rewards program (+ Merkl distributor) that is distributed directly by Ethena (Referral). See https://app.ethena.fi/joinUSDehas a "custom" rewards program that is distributed directly by Kernel DAO (Kernel Points). See https://kerneldao.gitbook.io/kernel/getting-started/kernel/kernel-points-guide
- ACI Merit Program: This is a program initiated and implemented by the Aave Chan Initiative (ACI)
- Merkl: This is a program initiated by the Aave DAO and implemented by Merkl. An integration guide can be seen here: https://docs.merkl.xyz/earn-with-merkl/earning-with-merkl
Recommendation
Kiln should contact the Aave DAO to
- confirm that the old "Aave Reward Controller" system has been deprecated. If this is confirmed the existing
AaveV3Merklmodule can be removed from the codebase - list and integrate all the other reward distribution systems that are used for the existing market
Kiln
Fixed by commit
65dcdd174c20eea6fafb5c4e91cc9dca1b2f5c0eWe decided to remove the Aave V3 module and focus on Merkl rewards only at launch via the Interceptor pattern.
Spearbit
Fixed. Note: Aave V3 rewards are not distributed only via Merkl but also via other custom distribution channels.
The AaveV3Merkl must be fully refactored
State
Severity
- Severity: Low
Submitted by
StErMi
Description
The current implementation of the
AaveV3Merklcontains multiple critical issues and wrong assumptions that need to be fixed- The
_rewardAssetpassed torewardsController.claimAllRewardsis NOT the underlying token (likeUSDC) but theATokenorVToken(in our case it will always be anATokengiven that the protocol never borrows and only supplies). This wrong assumption does not only influence the interaction with the reward controller but the whole logic that needs a full refactor of the module itself - For a single asset (
AToken) you can receive multiple rewards, not a single one - For a single asset (
AToken) you could receive as a reward (part of the multiple rewards accrued) the asset itself (anAToken)
Recommendation
⚠️ NOTE: it's high likely that the "Aave Reward Controller" reward distribution system has been already discontinued and won't be used anymore by Aave. If that's the case, the whole module can be just removed from the codebase without applying any fixes.
Kiln must refactor the whole
AaveV3Merklto properly integrate with the Aave Reward system. Below are some suggestions that can be used to kickstart the refactoring.The
AaveV3MerklModule.execfunction is executed in this way: someone callsModulesManager.exec(aaveVehicle, marklModuleId, merklRewardDistributionCampaignData). This will executeaaveVehicle.exec(marklModuleId, merklRewardDistributionCampaignData)which will execute inDELEGATECALLmodeAaveV3MerklModule.exec(marklModuleId, merklRewardDistributionCampaignData). This means that theAaveV3MerklModule.execlogic is executed with theAaveV3Vehiclecontext.- Get the vehicle's underlying asset by executing
address baseVaultAsset = _asset() - Get the corresponding
aTokenby calling(address _aToken,,) = _poolDataProvider.getReserveTokensAddresses(baseVaultAsset) - Execute
(address[] memory rewardsList, uint256[] memory claimedAmounts) = rewardsController.claimAllRewardsToSelf([baseVaultAsset]) - Iterate over the returned list to create, for each
rewardsList[i]a Merkl Distribution ifclaimedAmounts[i] > 0
Relative to the Merkl distribution:
There are some important things to note and be aware of when
distributionCreator.createCampaignis executed (or the bulk version of the function calledcreateCampaigns)- The
AaveV3Vehiclemust have SIGNED thedistributionCreatoragreement before callingdistributionCreator.createCampaign.AaveV3Vehiclemust execute (one time)distributionCreator.acceptConditions()otherwisedistributionCreator.createCampaignwill revert. - Check if
rewardsList[i]has been whitelisteddistributionCreator.rewardTokenMinAmounts(rewardsList[i]) > 0. What should the Module do if it has not been whitelisted? Avoid calling therewardsController.claimAllRewardsToSelffor that reward? - Check if the reward amount is enough to satisfy the requirement
claimedAmounts[i] * HOUR >= distributionCreator.rewardTokenMinAmounts(rewardsList[i]) * newCampaign.duration. What should the Module do ifclaimedAmounts[i]is not enough? Avoid calling therewardsController.claimAllRewardsToSelffor that reward?
⚠️ NOTE 2: Merkl could take a fee on the distributed rewards. The final amount of rewards received by the users could be less than
claimedAmounts[i]⚠️ NOTE 1: it's possible to claim, for an
assetjust a specific reward. The logic can be changed, it would be more complex but it's not so problematic.Kiln
Fixed by commit
65dcdd174c20eea6fafb5c4e91cc9dca1b2f5c0eWe decided to remove the Aave V3 module and focus on Merkl rewards only at launch via the Interceptor pattern.
Spearbit
Fixed.
Incorrect Asset Accounting if Vehicle Is Morpho Market Fee Recipient
State
Severity
- Severity: Low
Submitted by
Alireza Arjmand
Description
The
MorphoBlueVehiclecalculates its total assets using Morpho’s helper and relies onMorphoBalancesLib.expectedSupplyAssets. However, Morpho explicitly documents that this function is incorrect when the queried address is the marketfeeRecipient:/// @dev Warning: Wrong for `feeRecipient` because their supply shares increase is not taken into account.In Morpho Blue, protocol fees are realized by minting additional supply shares to the
feeRecipientduring interest accrual. This occurs in the core Morpho contract when fees are applied to the market’s supply shares, sending them to thefeeRecipientaddress.Because
expectedSupplyAssetsdoes not correctly account for these fee shares, querying it for thefeeRecipientwill produce an incorrect asset value.Recommendation
Ensure that the vehicle address can never be configured as the Morpho market
feeRecipient.Also consider:
- Document this as a hard integration invariant.
- Enforce it during initialization by verifying that the market’s
feeRecipientis not the vehicle address. It must be noted, that even if this check passes, owner can later change thefeeRecipient.
Kiln
This is unlikely to happen and as the
feeRecipientcan change, we will document it and not enforce it at initialisation. Fixed by https://github.com/kilnfi/railnet/pull/375/changes/6851b747130f3df87001f7f4d5c6a22e8e95ab56Spearbit
Verified fix. The documentation now explicitly mentions this behaviour.
Effects of rounding on both the vault's shares and underlying protocol shares
State
Severity
- Severity: Low
Submitted by
StErMi
Description
This is research I've done to explore the effects that the rounding directions can have on both the user's vault's share and the underlying protocol's share. In this specific case I have used Aave V3 as the underlying protocol (and vehicle) given that it's the one I'm more familiar with.
In a vehicle the user can perform two operations: deposit assets and redeem the vault's share (at least for the Single Asset type of Vehicles). The underlying protocol will (mostly) always defend the protocol's itself by rounding "against" the user (and in favor of the protocol):
- During a
DEPOSIToperation (supply on Aave) Aave V3 will round DOWN the amount of shares minted to the Vehicle - During a
REDEEMoperation (withdraw on Aave, Aave V3 does not have aredeemfunction) Aave V3 will round UP the amount of shares to be burned. - The
balanceOfoperation will round DOWN the balance of the user (the vehicle in our case)
On the Vehicle's side, the Vehicle will follow these rounding rules:
- During a
DEPOSIToperation the Vehicle will round DOWN the amount of shares minted to the user:SharesLib.convertToShares(..., Math.Rounding.Floor) - During a
REDEEMoperation the Vehicle will round DOWN the amount of assets to be withdrawn by redeeming the specified shares.SharesLib.convertToAssets(..., Math.Rounding.Ceil)
⚠️ Note: the amount of shares minted / burned by the protocol to/from the user during a
DEPOSITorREDEEMoperation does not "match" the one minted/burned by the underlying protocol's as a result of the operation. The Vehicle use it's own "logic" implemented in theSharescontract:function convertToAssets( uint256 shares, uint8 sharesDecimals, uint8 assetDecimals, uint256 totalSupply, uint256 totalAssets, Math.Rounding rounding) internal pure returns (uint256 assets) { if (totalSupply == 0) { return scale(shares, sharesDecimals, assetDecimals); } return downscaleAssets( Math.mulDiv( upscaleShares(shares, sharesDecimals, assetDecimals), upscaleAssets(totalAssets, sharesDecimals, assetDecimals), upscaleShares(totalSupply, sharesDecimals, assetDecimals), rounding ), sharesDecimals, assetDecimals );} function convertToShares( uint256 assets, uint8 sharesDecimals, uint8 assetDecimals, uint256 totalSupply, uint256 totalAssets, Math.Rounding rounding) internal pure returns (uint256 shares) { if (totalSupply == 0) { return scale(assets, assetDecimals, sharesDecimals); } if (totalAssets == 0) { revert ZeroTotalAssets(); } return downscaleShares( Math.mulDiv( upscaleAssets(assets, sharesDecimals, assetDecimals), upscaleShares(totalSupply, sharesDecimals, assetDecimals), upscaleAssets(totalAssets, sharesDecimals, assetDecimals), rounding ), sharesDecimals, assetDecimals );}Because there's not a 1:1 match between the Vehicle's shares minted/burned to/from the user and the underlying protocol's shares minted/burned to/from the Vehicle (on behalf of the user) the reasoning around the rounding becomes more complex.
The result of each operation and the rounding effect depends on the current exchange rate (on the vehicle) and the exchange rate on the underlying protocol (in the case of Aave V3, we use an index, not a "real" exchange rate).
By Fork testing and fuzzing we can see that there are both cases where the user operation could erode (LOSS) the Vehicle's original underlying balance in the protocol or it could even make the Vehicle EARN because the user is leaving money on the plate.
The Fork test I have created performs 1 single deposit and multiple chunked redeem. What I can assume given how the Vehicle's share logic works and how Aave's share logic works, is that
- when the Vehicle's LOSS is mainly because Aave is rounding against the Vehicle and the Vehicle is NOT rounding against the user
- when the Vehicle's EARN is because Aave is NOT rounding against the Vehicle and the Vehicle is rounding against the user
It's important to note that every loss or earn consequences that influence the Vehicle are also influencing the existing suppliers' balances. When the Vehicle "earn" the existing suppliers' share value increases, when the Vehicle "lose" the existing suppliers' share value decreases.
The test
There are two fuzz tests implemented that internally apply the same logic but they try to prove two different things:
testStudyEarntry to prove that the Vehicle can EARN more thanthresholdwei at the end of the testtestStudyLosstry to prove that the Vehicle can LOSE more thanthresholdwei at the end of the test
Both the tests will always perform the following actions:
- deploy the Vehicle with an initial deposit of 1 unit of asset (in this case 1 USDC)
u1deposit the fuzzedinitialDepositamount of USDCu1will redeem in loopsplittimes an amount of shares equal to$vehicle.balanceOf(u1) / split. The last iteration will redeem the whole remaining balance- Depending on the
proofTypeit will try to assert that the Vehicle is indeed earning/losingthresholdof wei.
// SPDX-License-Identifier: BUSL-1.1pragma solidity >=0.8.33; import { STEAMSingleAssetTester, IERC20Metadata, AaveV3VehicleFactory, Asset, Query, State, Mode, EstimationType, ErrorLib, IERC20, Tester, Route} from "test/vehicles/aave_v3/AaveV3Vehicle.t.sol";import {IPoolAddressesProvider} from "src/vehicles/aave_v3/interfaces/IPoolAddressesProvider.sol";import {IPoolDataProvider} from "src/vehicles/aave_v3/interfaces/IPoolDataProvider.sol";import {ForkUtils} from "test/test_utils/ForkUtils.sol";import {IExternalAccessControl, ExternalAccessControl} from "src/common/ExternalAccessControl.sol";import {FeeManager} from "src/vehicles/base/FeeManager.sol";import {ModulesManager} from "src/vehicles/base/ModulesManager.sol";import {Vm} from "@forge-std/Test.sol";import {CoreFactory, ICreateX} from "src/factories/CoreFactory.sol";import {Roles} from "src/libs/Roles.sol";import {AccessControlFactory} from "src/factories/common/AccessControlFactory.sol";import {ModulesManagerFactory} from "src/factories/vehicles/ModulesManagerFactory.sol";import {AaveV3MerklModule} from "src/vehicles/aave_v3/modules/AaveV3Merkl.mod.sol";import {SafeERC20} from "@openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";import {IDistributionCreator} from "src/vehicles/base/interfaces/IDistributionCreator.sol";import {BaseVehicleErrors} from "src/vehicles/base/abstracts/BaseVehicleErrors.sol";import 'forge-std/console.sol'; interface IAToken { function balanceOf(address account) external view returns (uint256); function scaledBalanceOf(address user) external view returns (uint256);} interface IPool { function deposit(address asset, uint256 amount, address onBehalfOf, uint16 referralCode) external;} contract _SErosionStudySingleTest is STEAMSingleAssetTester, ForkUtils { using Tester for Query; using SafeERC20 for IERC20; address internal constant BURN_ADDRESS = address(0x000000000000000000000000000000000000dEaD); address internal constant $USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48; address internal constant $DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F; address internal constant $poolAddressesProvider = 0x2f39d218133AFaB8F2B819B1066c7E434Ad94E9e; address internal constant $distributionCreator = 0x8BB4C975Ff3c250e0ceEA271728547f3802B36Fd; address internal $pool; address internal $poolDataProvider; ExternalAccessControl internal $accessControl; ModulesManager internal $modulesManager; Vm.Wallet private $admin; CoreFactory _coreFactory; uint256 salt; function setUp() external { _startFork("MAINNET"); $asset = IERC20Metadata($USDC); $admin = vm.createWallet("admin"); _coreFactory = _deployCoreFactory(ICreateX(0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed)); ExternalAccessControl.RoleAttribution[] memory _roles = new IExternalAccessControl.RoleAttribution[](5); _roles[0] = IExternalAccessControl.RoleAttribution({role: Roles.EXEC, account: $admin.addr}); _roles[1] = IExternalAccessControl.RoleAttribution({role: Roles.MODULE_MANAGER, account: $admin.addr}); _roles[2] = IExternalAccessControl.RoleAttribution({role: Roles.CANCEL_MODULE, account: $admin.addr}); _roles[3] = IExternalAccessControl.RoleAttribution({role: Roles.UPDATE_TIMELOCK, account: $admin.addr}); _roles[4] = IExternalAccessControl.RoleAttribution({role: Roles.VEHICLE_ALLOW, account: $admin.addr}); ($accessControl,) = _deployAccessControl( _coreFactory, AccessControlFactory.SpawnParams({ initialDelay: 0, initialDefaultAdmin: makeAddr("access_control_admin"), initialRoles: _roles, deploymentSalt: keccak256("vehicle_access_control") }) ); ($modulesManager,) = _deployModulesManager( _coreFactory, ModulesManagerFactory.SpawnParams({ initialTimelock: 0, accessControl: $accessControl, deploymentSalt: "MODULES_MANAGER" }) ); ($vehicle,) = _deployAaveV3Vehicle( _coreFactory, AaveV3VehicleFactory.SpawnParams({ asset: address($asset), poolAddressesProvider: $poolAddressesProvider, accessControl: $accessControl, feeManager: FeeManager(address(0)), modulesManager: $modulesManager, querySalt: keccak256("INITIAL_DEPOSIT_QUERY"), deploymentSalt: keccak256("DEPLOYMENT_SALT"), initialDepositSize: _unit(1), initialExpectedSupply: 1e18 }) ); vm.startPrank(makeAddr("access_control_admin")); $accessControl.setRolePublic(Roles.VEHICLE_STEAM, true); vm.stopPrank(); $pool = IPoolAddressesProvider($poolAddressesProvider).getPool(); $poolDataProvider = IPoolAddressesProvider($poolAddressesProvider).getPoolDataProvider(); $multiDepositRangeMin = 4; $multiDepositRangeMax = 16; vm.startPrank($admin.addr); IDistributionCreator($distributionCreator).acceptConditions(); vm.stopPrank(); } enum ProofType { LOSS, EARN } function testStudyEarn(uint256 initialDeposit, uint256 split) public { initialDeposit = bound(initialDeposit, _unit(1), _unit(1_000_000)); split = bound(split, 1, 1000); execTest(ProofType.EARN, 10, initialDeposit, split); } function testStudyLoss(uint256 initialDeposit, uint256 split) public { initialDeposit = bound(initialDeposit, _unit(1), _unit(1_000_000)); split = bound(split, 1, 1000); execTest(ProofType.LOSS, 10, initialDeposit, split); } function execTest(ProofType proofType, uint256 threshold, uint256 initialDeposit, uint256 split) public { address es = makeAddr("existingSupplier"); _deposit(es, _unit(1)); address u1 = makeAddr("u1"); (uint256 aTokenScaledBalanceStart, uint256 aTokenBalanceStart) = fetchVehicleBalances(address($asset)); uint256 vShareDeposit = _deposit(u1, initialDeposit); uint256 u1Shares = $vehicle.balanceOf(u1); uint256 redeemAmount = u1Shares / split; for( uint256 i = 0; i < split; i++ ) { (uint256 aTokenScaledBalanceBeforeRedeem, uint256 aTokenBalanceBeforeRedeem) = fetchVehicleBalances(address($asset)); if( i == split - 1) redeemAmount = $vehicle.balanceOf(u1); uint256 tokenWithdrawn = _redeem(u1, redeemAmount); (uint256 aTokenScaledBalanceAfterRedeem, uint256 aTokenBalanceAfterRedeem) = fetchVehicleBalances(address($asset)); console.log('v token burned. ', aTokenScaledBalanceBeforeRedeem-aTokenScaledBalanceAfterRedeem); console.log('u token burned. ', redeemAmount); console.log('v token withdrawn', aTokenBalanceBeforeRedeem-aTokenBalanceAfterRedeem); console.log('u token withdrawn', tokenWithdrawn); } (uint256 aTokenScaledBalanceEnd, uint256 aTokenBalanceEnd) = fetchVehicleBalances(address($asset)); console.log('vb ini', aTokenBalanceStart); console.log('vb end', aTokenBalanceEnd); console.log('ub ini', initialDeposit); console.log('ub end', IERC20(address($asset)).balanceOf(u1)); // how much has changed the existing suppliers balance in value? console.log('es value', estimateUserBalance(es)); if( aTokenBalanceEnd > aTokenBalanceStart ) { // Vehicle is EARNING console.log('v EARN', aTokenBalanceEnd-aTokenBalanceStart); console.log('u LOSS', initialDeposit-IERC20(address($asset)).balanceOf(u1)); if( proofType == ProofType.EARN ) assertLt(aTokenBalanceEnd-aTokenBalanceStart, threshold); } else if ( aTokenBalanceEnd < aTokenBalanceStart ) { // Vehicle is LOSING console.log('v LOSS', aTokenBalanceStart-aTokenBalanceEnd); console.log('u LOSS', initialDeposit-IERC20(address($asset)).balanceOf(u1)); if( proofType == ProofType.LOSS ) assertLt(aTokenBalanceStart-aTokenBalanceEnd, threshold); } else { // no-loss-earn scenario } } function estimateUserBalance(address user) public returns (uint256) { uint256 userVehicleBalance = $vehicle.balanceOf(user); if(userVehicleBalance == 0) return 0; Asset[] memory assets = new Asset[](1); assets[0] = Asset({asset: address($vehicle), value: userVehicleBalance}); Asset[] memory output = $vehicle.estimate(assets, Mode.REDEEM, EstimationType.OUTPUT); return output[0].value; } function _deposit(address user, uint256 amount) public returns (uint256 output) { // uint256 _depositAmount = _unit(amount); uint256 _depositAmount = amount; deal(address($asset), user, _depositAmount); vm.startPrank(user); IERC20(address($asset)).approve(address($vehicle), _depositAmount); Asset[] memory _depositAssets = new Asset[](1); _depositAssets[0] = Asset({asset: address($asset), value: _depositAmount}); Asset[] memory _depositOutput = new Asset[](1); _depositOutput[0] = Asset({asset: address($vehicle), value: 0}); Query memory _query = Query({ owner: user, receiver: user, input: _depositAssets, mode: Mode.DEPOSIT, salt: bytes32(salt++), output: _depositOutput, data: "" }); $vehicle.create(_query); (State newState, Asset[] memory assets) = $vehicle.unlock(_query); vm.stopPrank(); return assets[0].value; } function _redeem(address user, uint256 amount) public returns (uint256 output) { Asset[] memory _assets = new Asset[](1); _assets[0] = Asset({asset: address($vehicle), value: amount}); Asset[] memory _output = new Asset[](1); _output[0] = Asset({asset: address($asset), value: 0}); Query memory _query = Query({ owner: user, receiver: user, input: _assets, mode: Mode.REDEEM, salt: bytes32(salt++), output: _output, data: "" }); vm.startPrank(user); $vehicle.create(_query); (State newState, Asset[] memory assets) = $vehicle.unlock(_query); vm.stopPrank(); return assets[0].value; } function fetchVehicleBalances(address underlying) public returns (uint256, uint256) { (address _aToken,,) = IPoolDataProvider($poolDataProvider).getReserveTokensAddresses(underlying); uint256 aTokenScaledBalance = IAToken(_aToken).scaledBalanceOf(address($vehicle)); uint256 aTokenBalance = IAToken(_aToken).balanceOf(address($vehicle)); return (aTokenScaledBalance, aTokenBalance); } }Recommendations
Kiln should:
- Deploy enough initial assets into the Vehicle (when the vehicle is initially deployed) to avoid any possible "erosion" attack vector to be performed by cycling the redeem operation that could reduce the Vehicle's initial total assets in the underlying protocol
- Monitor the Vehicle's "erosion" issue during the Vehicle's lifecycle and have pre-defined plan to eventually donate on behalf of the vehicle to reimburse the loss in share price that the existing suppliers could incur.
- Document and disclose the "erosion" behavior: the user's actions could erode the vehicle's total assets in the underlying protocol, reducing, as a consequence, the existing suppliers share's value.
Kiln
Fixed by commit
152376e7b86c94009f3c2817ee9b067a03bb5b85.Spearbit
Kiln has introduced the
AssetRegistrycontract that will- manage a whitelist of assets that the vehicles can pick from when deployed
- manage, for each asset, the minimum asset's amount to be deployed and deposited on the vehicle's deployment
Kiln has also documented and acknowledged the "erosion" issue described by the Finding
Informational44 findings
Use of deprecated contract comparison operators
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The solidity 0.8.31 version release deprecates the comparison of contract type variables.
Comparison of variables of contract type is deprecated and scheduled for removal. Use an explicit cast to address type and compare the addresses instead.
Reference:
Recommendation
Cast the contract type variables to address before comparison.
Kiln
Fixed in commit cdfb38f.
Spearbit
Fixed. The contract type comparisons have been replaced by address comparisons.
STEAM specifications precision about redeem optional output
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The STEAM specifications state the following about the output asset of a query:
For
DEPOSIToperations, this optionally specifies the output asset the user expects to receive as shares or other tokens resulting from the deposit. ForREDEEMoperations, this specifies the desired output asset the user wishes to receive from the Vehicle upon redemption.The implementation of
SingleAssetBaseVehicleshows that bothDEPOSITandREDEEMoperations use optional outputs.Recommendation
Add details in the specifications that the asset output is also optional for
REDEEMoperations.For
DEPOSIToperations, this optionally specifies the output asset the user expects to receive as shares or other tokens resulting from the deposit. ForREDEEMoperations, this optionally specifies the desired output asset the user wishes to receive from the Vehicle upon redemption.Kiln
Fixed in commit 555323a.
Spearbit
Fixed. The specifications section has been refactored and now correctly mentions that both deposit and redeem operations use the
outputarray as an optional field.Scaling factor calculation in _previewOnOperations is prone to edge-cases
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The
FeeManager._previewOnOperationsfunction calculates and uses a scaling factor in case_totalFeeAssets > _maxFees.In such calculation, if
totalFeeAssetsis greater than10 ** assetDecimals * _maxFeesthen_scalingFactor = 1. After this,_managementFeeAssets = _managementFeeAssets * 1 / 10 ** assetDecimals. In extreme edge-cases, this could still result in_totalFeeAssets > currentTotalAssetswhich would later revert.function _previewOnOperations( // ... uint256 _totalFeeAssets = _performanceFeeAssets + _managementFeeAssets; // Cap fee assets to prevent underflow when cache is uninitialized (lastTimestamp == 0) uint256 _maxFees = Math.mulDiv(currentTotalAssets, ConstantLib.BPS_MAX - 1, ConstantLib.BPS_MAX); if (_totalFeeAssets > _maxFees) { // _performanceFeeAssets and _managementFeeAssets are capped proportionally uint256 _scalingFactor = Math.mulDiv(10 ** assetDecimals, _maxFees, _totalFeeAssets, Math.Rounding.Ceil); // @audit _performanceFeeAssets = Math.mulDiv(_performanceFeeAssets, _scalingFactor, 10 ** assetDecimals, Math.Rounding.Floor); _managementFeeAssets = Math.mulDiv(_managementFeeAssets, _scalingFactor, 10 ** assetDecimals, Math.Rounding.Floor); _totalFeeAssets = _performanceFeeAssets + _managementFeeAssets; } if (_totalFeeAssets != 0) { uint256 _totalAssetsWithoutFees = currentTotalAssets - _totalFeeAssets; // @audit could revertNote: This issue seems unlikely to be exploitable. However, the recommended fix erases the edge-case while providing more readability.
Recommendation
Consider removing the scaling factor calculation and simplifying
_performanceFeeAssetsand_managementFeeAssets.The following patch ensures that
_totalFeeAssetscan not be greater than_maxFeeswhile removing the scaling factor.diff --git a/src/vehicles/base/FeeManager.sol b/src/vehicles/base/FeeManager.solindex 3042b703..0ef8eb65 100644--- a/src/vehicles/base/FeeManager.sol+++ b/src/vehicles/base/FeeManager.sol@@ -493,11 +493,9 @@ contract FeeManager is IFeeManager, Interceptor, ReentrancyGuardUpgradeable, Ext uint256 _maxFees = Math.mulDiv(currentTotalAssets, ConstantLib.BPS_MAX - 1, ConstantLib.BPS_MAX); if (_totalFeeAssets > _maxFees) { // _performanceFeeAssets and _managementFeeAssets are capped proportionally- uint256 _scalingFactor = Math.mulDiv(10 ** assetDecimals, _maxFees, _totalFeeAssets, Math.Rounding.Ceil);- _performanceFeeAssets =- Math.mulDiv(_performanceFeeAssets, _scalingFactor, 10 ** assetDecimals, Math.Rounding.Floor);- _managementFeeAssets =- Math.mulDiv(_managementFeeAssets, _scalingFactor, 10 ** assetDecimals, Math.Rounding.Floor);+ _performanceFeeAssets = Math.mulDiv(_performanceFeeAssets, _maxFees, _totalFeeAssets, Math.Rounding.Floor);+ _managementFeeAssets = _maxFees - _performanceFeeAssets;+ // @audit or use: _managementFeeAssets = Math.mulDiv(_managementFeeAssets, _maxFees, _totalFeeAssets, Math.Rounding.Floor); _totalFeeAssets = _performanceFeeAssets + _managementFeeAssets; } if (_totalFeeAssets != 0) {Kiln
Fixed in commit a75c49c.
Spearbit
Fixed. Recommended patch has been applied.
Merkl Trust Assumptions
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Trust assumptions for usage of Merkl via the reward distribution system are listed below, it is recommended to keep these in mind when utilizing the protocol:
- Merkl will assign the same campaign ID to campaigns that differ only in total amount while keeping all other parameters identical, this will result in the revert of the second transaction that lands.
- Both the contract and the caller are expected to be properly whitelisted by Merkl before interacting with the system.
- Merkl enforces a minimum threshold on the total campaign amount to avoid economically insignificant campaigns.
- Merkl will enforce a minimum threshold on the distributed amount to prevent negligible distributions.
- Users themselves and whitelisted operators approved by Merkl are able to claim rewards on behalf of users.
Natspec issues
State
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Description
Several natspec comments and documentation strings are inaccurate, incomplete, or misleading:
- FreezablePausableBeacon.sol?lines=153,153: The
getStatus()note states "implementationAddress may be inaccessible if the beacon is paused", but the paused state has no impact on retrieving the$implementationstorage variable. Only theimplementation()view function is affected by the pause. - IFreezablePausableBeacon.sol?lines=48,48: The
BeaconFrozen()error comment states it is "thrown when attempting to upgrade a frozen beacon". In practice, the error is also thrown onpause()andpauseFor()calls, and when attempting to freeze an already frozen beacon. - IFreezablePausableBeacon.sol?lines=58,58: The
ExpectedPause()error is declared but never used. - IFreezablePausableBeacon.sol?lines=65,65: The
upgradeTonatspec states "Can only be called when not frozen and not paused", but the implementation only enforceswhenNotFrozen. The "and not paused" mention is incorrect and should be removed. - IExternalAccessControl.sol?lines=80,94: The natspec for
grantScopedRole()andrevokeScopedRole()states "Can only be called by accounts with DEFAULT_ADMIN_ROLE". The actual modifier isonlyRole(getRoleAdmin(role)), which is not necessarilyDEFAULT_ADMIN_ROLE. - IVehicle.sol?lines=92,92: The
error()function natspec states it "Gets the error encountered by a query that led to RECOVERING or REJECTED state", but in practiceBaseVehicle.erroronly returns an error when the state isREJECTED, notRECOVERING. - Asset.sol?lines=65,65: The comment for output matching describes the opposite of the actual logic.
- Error.sol?lines=39,39: The
ZeroLength()error comment is a copy-paste of theZeroValue()comment. It should read "The error thrown when the length is 0." - Error.sol?lines=68,68: Typo: "Raise" should be "Raised".
- Factory.sol?lines=146,146: The return parameter of
deployContractis namedvehiclebut should becontract. - Factory.sol?lines=248,248: The
generateVehicleSymbolnatspec does not document thecountparameter. - Rejection.sol?lines=66,66: The natspec for
rejectWithInvalidInputandrejectWithInvalidOutputstates that current inputs/outputs are returned for comparison, but only the query inputs/outputs are encoded. - Roles.sol?lines=103,103: Typo: "preform" should be "perform".
- Shares.sol?lines=23,23: The
ZeroTotalAssets()error comment states it is thrown "during conversion", but it is only thrown duringconvertToShares. - Vehicle.sol?lines=163,163: The
@returncomment ends with "false" and should read "false otherwise". - BaseVehicleChecks.sol?lines=49,49: Comment says "Prevents calls when the vehicle is not in the expected state" but it should say "when the query is not in the expected state".
- BaseVehicleErrors.sol?lines=35,35: The errors
InvalidReceiver(),InvalidOwner(), andInvalidMode(Mode expected, Mode actual)are declared but never used. - BaseVehicleInitialization.sol?lines=71,71: The
forbiddenAddressesparam comment says "forbidden to create queries" but should say "forbidden to create or receive queries". - BaseVehicleQueryStateSetter.sol?lines=89,89: Comment references
createas a function that can transition toREJECTED, butcreate()no longer transitions toREJECTEDper the current specs. - FeeManager.sol?lines=50,50:
FeeManagermentionsredeemVehicleSharesbut it can't be found in the contract. - FeeManager.sol?lines=78,78: Comment says "ModulesManager", but it is set on the "FeeManager"
- FeeManager.sol?lines=449,449: Comment indicates "Performance fee is calculated on remaining profit (after management fee deduction)". However this is not accurate anymore. Performance fees are taken on top of management fees.
- FeeManager.sol?lines=492,492: remove the mentioned comment because it is referencing a scenario that does not exist anymore.
- IBaseVehicle.sol?lines=34,36: Comment uses the outdated STEAM states "Empty → Pending → Accepted → Processing → Unlocking → Settled".
- IFeeManager.sol?lines=57,57: Comment mentions "Sets the maximum allowed fees" but it actually sets the fees and not the maximum.
- IFeeManager.sol?lines=81,81: The comment mentions that the state is being modified, but there is no state being updated and only an event is being emitted.
- ExternalAccessControl.sol?lines=158,158: The
ExternalAccessControl.setRolePublicfunction references the wrong function. The scopedDEFAULT_ADMIN_ROLErole can be made public by using thesetRolePublicand not thesetScopedRolePublic. Rewrite the natspec comment correctly. Example: "However, scopedDEFAULT_ADMIN_ROLEcan be made public throughsetRolePublic(). If passed an encoded role directly the code has no way to differentiate and prevent a scopedDEFAULT_ADMIN_ROLEbeing made public." - Factory.sol?lines=48,49: The
Factory.InitialDepositParamsnatspec is outdated. Theassetandamountparameters have been replaced byinputAssetandoutputAsset. Update it accordingly. Consider also to renameamounttoinputAmountgiven that it will exclusively be used for theinputparameter of theQuery. - Vehicle.sol?lines=74,157: Consider improving the natspec of the
convertToAssets,convertToShares,estimateSharesandestimateAssetsfunctions in theVehicleto explicitly disclose which fees are applied in the underlyingvehicle.estimatecall. - BaseVehicleChecks.sol?lines=34,36: The
BaseVehicleChecks._onlyRoleWhenEnabledfunction's natspec should explicitly disclose with a@devcomment that the function won't perform any role check if the Access Control has not been configured. Consider also renaming the function to be explicit in this behavior. - BaseVehicle.sol?lines=670,670 + BaseVehicle.sol?lines=699,699: Both the
_maxDepositWithFeesand_maxRedeemWithFeesfunctions natspec in theBaseVehiclecontract wrongly use the term "minting". These areviewfunction that don't mint any fee shares but "account" them into the calculation. Replace the "minting" term with the "accounting" one.
Recommendation
Review and correct the cited natspec comments and documentation strings to accurately reflect the implemented behavior. Remove unused error declarations.
Kiln
Fixed by commit
d1392bdd71ddffe4d612379368c44aa9e7a39df8.Spearbit
Fixed. Additional fixes provided in the commit commit
8fa112d7c9fe8fdf55529a562feb49287a8c3e33.[📚 Research] AAVE Reward Distribution
State
Severity
- Severity: Informational
Submitted by
StErMi
"Custom" reward programs
Example 1:
Wrapped eETHWrapped eETHhas a "custom" rewards program that is distributed directly by Ether.fi (Loyalty Points). See https://etherfi.gitbook.io/etherfi/getting-started/loyalty-pointsExample 2:
USDeUSDehas a "custom" rewards program (+ Merkl distributor) that is distributed directly by Ethena (Referral). See https://app.ethena.fi/joinExample 3:
rsETHrsETHhas a "custom" rewards program that is distributed directly by Kernel DAO (Kernel Points). See https://kerneldao.gitbook.io/kernel/getting-started/kernel/kernel-points-guideACI Merit Program
This is a program initiated and implemented by the Aave Chan Initiative (ACI). Aave Labs does not guarantee the program and accepts no liability. Learn more https://apps.aavechan.com/merit
Merkl
This is a program initiated by the Aave DAO and implemented by Merkl. Aave Labs does not guarantee the program and accepts no liability. An integration guide can be seen here: https://docs.merkl.xyz/earn-with-merkl/earning-with-merkl
⚠️ IMPORTANT NOTE: it seems that each Merkl distribution campaign has its own "requirements" which could be more than just "supply the liquidity and hold it"
AAVE Rewards Controller (Mainnet study)
Mainnet AAVEV3 Default Incentives Controller: https://etherscan.io/address/0x8164Cc65827dcFe994AB23944CBC90e0aa80bFcb
Asset List
These are the assets for which the user will get some rewards when they hold them in their balance. ⚠️ IMPORTANT NOTE: These assets could be both
ATokenorVToken- Aave Ethereum ETHx (aEthETHx)
- Aave Ethereum Lido WETH (aEthLidoWETH)
- Aave Ethereum Lido wstETH (aEthLidowstETH)
- Aave Ethereum USDS (aEthUSDS)
- Aave Ethereum Lido USDC (aEthLidoUSDC)
Reward List
⚠️ IMPORTANT NOTE: the reward of an asset could be the asset's itself (usually an
AToken)- Aave Ethereum ETHx (aEthETHx)
- (❗️ ENDED) Stader (SD)
- Aave Ethereum Lido WETH (aEthLidoWETH)
- (❗️ ENDED) Aave Ethereum Lido WETH (aEthLidoWETH)
- Aave Ethereum Lido wstETH (aEthLidowstETH)
- (❗️ ENDED) Wrapped liquid staked Ether 2.0 (wstETH)
- (❗️ ENDED) Aave Ethereum Lido wstETH (aEthLidowstETH)
- Aave Ethereum USDS (aEthUSDS)
- ( ✅ ACTIVE UNTIL 04 March 2026) Aave Ethereum USDS (aEthUSDS)
- Aave Ethereum Lido USDC (aEthLidoUSDC)
- (❗️ ENDED) Aave Ethereum Lido wstETH (aEthLidowstETH)
Kiln
Fixed by commit
65dcdd174c20eea6fafb5c4e91cc9dca1b2f5c0eWe decided to remove the Aave V3 module and focus on Merkl rewards only at launch via the Interceptor pattern.
Spearbit
Fixed. Note: Aave V3 rewards are not distributed only via Merkl but also via other custom distribution channels.
Account.performCall incorrectly reverts with Unauthorized error
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
Account.performCallreverts with theUnauthorizederror through the following code.if (msg.sender != $owner) { revert ErrorLib.Unauthorized(msg.sender, $owner); }The variable ordering is incorrect as
Unauthorizederror definition is:error Unauthorized(address expectedCaller, address caller);Recommendation
Swap the two variables.
if (msg.sender != $owner) {- revert ErrorLib.Unauthorized(msg.sender, $owner);+ revert ErrorLib.Unauthorized($owner, msg.sender); }Kiln
Fixed in commit bc078b8.
Spearbit
Fixed. The variables have been swapped in the
Unauthorizederror.ExternalRBAC initializers are not disabled
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The
ExternalRBACcontract is inherited by non-upgradeable contracts. However, it inheritsExternalRBACUpgradeablewhich inheritsInitializable.The initializers are not disabled while the contract is not upgradeable.
Recommendation
Add a call to
_disableInitializers()inExternalRBAC.constructor.Kiln
Fixed in commit 9f9e388.
Spearbit
Fixed. The
_disableInitializersfunction is now called in constructor.isCategory incorrectly reverts instead of returning false
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The
isCategoryfunction in theVehiclelibrary reverts when there is no deposit route, no redeem route or when the number of deposit routes does not match the number of redeem routes.Considering that the STEAM standard does not enforce such conditions and that future STEAM implementations could use this library, the
isCategoryfunction should not revert on these conditions./// @dev Checks if a vehicle belongs to a specific category based on its routes. /// For SingleAsset category, validates that vehicle has routes for single asset deposit and single asset redeem. /// @param vehicle The vehicle contract to check. /// @param testedCategory The category to test against. /// @return True if the vehicle matches the category, false otherwise. function isCategory(IVehicle vehicle, VehicleCategory testedCategory) internal view returns (bool) { (Route[] memory _depositRoutes, Route[] memory _redeemRoutes) = vehicle.routes(); if (_depositRoutes.length == 0 || _redeemRoutes.length == 0 || _depositRoutes.length != _redeemRoutes.length) { revert InvalidRouteLengths(_depositRoutes, _redeemRoutes); }Recommendation
Consider returning
falseinstead of reverting inisCategory.Kiln
Fixed in commit 84daa3f.
Spearbit
Fixed. The
falseboolean is now returned. The revert behavior should now be handled by the caller.Dead code
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The codebase contains several instances of unused code like custom errors, library functions, and role constants that are defined but never referenced anywhere in the project.
-
src/common/interfaces/IFreezablePausableBeacon.sol:59: TheExpectedPauseerror is defined in theIFreezablePausableBeaconinterface but is never used anywhere in the codebase. The companion errorEnforcedPause(line 56) is used, butExpectedPausehas no corresponding usage. -
src/libs/Asset.sol:58-62: TheAsset.getSingleAssetValueFromMemoryfunction is not used and can be removed. -
src/libs/Error.sol:78,83,86,90,130: The following errors defined inErrorLibare never used anywhere in the codebase:Rejected,Settled,OnlyDelegateCall,DuplicatedAsset, andInvalidInitialDepositAmount. -
src/libs/Factory.sol:122:FactoryLibdefines its ownFailedVehicleCreation(address factory, Query query)error. This error is never used — theFactoryLib.executeDepositfunction reverts withFailedVehicleUnlock, notFailedVehicleCreation. A differentFailedVehicleCreation(address factory)error is defined inICoreFactory.soland used inCoreFactory.sol; theFactoryLibvariant is a dead, conflicting duplicate. -
src/libs/Rejection.sol:45,58:RejectionLibprovides two helper functions (rejectWithMaxDepositTooLowandrejectWithMaxRedeemTooLow) that are never called. The actual capacity-exceeded rejection logic inBaseVehicle.sol(lines 209–213) reverts directly withErrorLib.MaxDepositTooLowandErrorLib.MaxRedeemTooLowrather than using theseRejectionLibencoders. -
src/libs/Roles.sol:76: TheFEE_MANAGER_REDEEM_VEHICLE_SHARESrole constant is defined inRolesbut is not referenced anywhere in the codebase for access control checks or role assignments. -
src/vehicles/base/abstracts/BaseVehicleErrors.sol:36,39,43: The errorsInvalidReceiver,InvalidOwner, andInvalidMode(Mode expected, Mode actual)are declared inBaseVehicleErrorsbut never used. The other errors defined in the same contract (InvalidCaller,InvalidState,InvalidRecipient) are actively used.
Recommendation
Dead code should be removed from the codebase where possible. Removing unused declarations reduces code size, lowers the risk of confusion for developers and auditors, and eliminates potential inconsistencies.
Kiln
Fixed in commit 0a93627 and commit 864d1c1.
Spearbit
Fixed. Dead code occurrences have been fixed by being deleted or being used.
setDeprecated allows to "undeprecate"
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The
FactoryBase.setDeprecatedfunction allows the authorized party to deprecate a factory. But this function also allows to cancel deprecation of a factory.A deprecation should not be supposed to be cancellable.
Moreover, no event is emitted during the execution of
setDeprecated.Recommendation
Either change the "deprecation" naming, or make the deprecation non-cancellable.
Kiln
Fixed in commit 2894381.
Spearbit
Fixed.
setDeprecatedhas been renamed todeprecateand does not allow to cancel a deprecation. Moreover, theDeprecated()event is now emitted.Query ID is calculated at each round loop
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
In multiple locations,
query.toId(address(this))is executed in multiple loop rounds but this value will not change.The locations are:
Recommendation
Compute
query.toId(address(this))outside of loops if possible.Kiln
Fixed in commit bde6415.
Spearbit
Fixed. The query ID is calculated once and cached to be reused later.
Requirements for resume() Implementation in Derived Vehicles
Severity
- Severity: Informational
Submitted by
Optimum
Description
The
BaseVehiclecontract defines theresume()function as a core part of the STEAM query lifecycle, delegating the specific execution logic to the internal_resume(). To ensure the economic integrity and share pricing of the vehicle, any derived contract or facet implementing the resumption logic must adhere to the following requirements:resume()must be the function that interacts with the underlying protocol and therefore drives the change intotalAssets()(either increase via deposit or decrease via redeem/withdraw).resume()must be the function that mints vehicle shares; otherwise, the vehicle share price will be calculated incorrectly.- Minting vehicle shares means
resume()should also include management and performance fees while calculating the shares to be minted. resume()should estimate the output for the user and validate the slippage. As part of this process, it may override the value offeesConfigIdthat was previously stored during the call tocreate().
Recommendation
Consider implementing realistic vehicle contracts that fully implement functions such as
resume()andrecover(). This would make it easier for both the development team and security researchers to reason about the correctness of the STEAM state machine—particularly the logic inBaseVehicle.create(), which may be affected by the eventual implementation details ofresume()andrecover().Kiln
Fixed in 6038c67.
Spearbit
Fixed by implementing the reviewer's recommendation.
Missing non-zero check for underlying protocol minted shares
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Optimum
Description
Across multiple vehicle implementations, the internal logic for handling deposits lacks a validation check to ensure that the number of underlying protocol shares to be minted is greater than zero, As we can see in the sample implementation of :
function _create_deposit(Query calldata query, Id qid, uint256 totalSupply_, uint256 totalAssets_) internal virtual returns (State, State[] memory, Asset[] memory) { ERC4626VehicleStore.Storage storage $ = ERC4626VehicleStore.getStorage(); uint256 _assetsToDeposit = query.getSingleInputValue(); IERC20 _inputAsset = IERC20(query.getSingleInputAddress()); // [1] Calculate vehicle shares to mint using current exchange rate uint256 _sharesToMint = _preview_deposit(_assetsToDeposit, totalSupply_, totalAssets_, Math.Rounding.Floor); // [2] Approve ERC4626 vault to spend the deposited assets IERC4626 _underlyingVault = $.underlyingVault; SafeERC20.forceApprove(_inputAsset, address(_underlyingVault), _assetsToDeposit); // [3] Deposit assets into ERC4626 vault _underlyingVault.deposit(_assetsToDeposit, address(this)); // [4] Mint vehicle shares to the vehicle itself (not yet to user) _mint(address(this), _sharesToMint); // [5] Store the vehicle share amount for the unlock phase $.values[qid].amountToUnlock = _sharesToMint; // [6] Transition to UNLOCKING state return _transition(qid, State.UNLOCKING, query.input); }The underlying protocol shares being received from the call to
_underlyingVault.deposit()are never checked to be non-zero. If a user provides an amount of assets that, after accounting for fees or rounding, results in zero shares (due to rounding errors or any potential share price manipulation in the underlying protocol itself), the vehicle may still proceed with pulling assets from the user. This leads to a state where the user loses capital without receiving any representation of ownership in return.Recommendation
_create_deposit()should be changed to support to calculate the protocol shares received from the underlying protocol (_underlyingVaultin our code snippet example) and revert in case this value is 0.Kiln
We acknowledge this issue.
Spearbit
Acknowledged.
Missing msg.sender in SpawnedExternalAccessControl Event
Severity
- Severity: Informational
Submitted by
Optimum
Description
The spawn function in
AccessControlFactoryutilizes a guarded salt to prevent front-running, but theSpawnedExternalAccessControlevent does not record the msg.sender. This deviates from the design ofCoreFactory, where theVehicleCreatedandContractCreatedevents explicitly include themsg.senderfor indexing and tracking.Recommendation
Update the
SpawnedExternalAccessControlevent signature to include an indexed deployer address and passmsg.senderduring emission.Kiln
Fixed in 84a13a42
Spearbit
Fixed by implementing the reviewer's recommendation.
Missing receive() function prevents Account from accepting ETH
Severity
- Severity: Informational
Submitted by
Optimum
Description
The
Accountcontract is designed to hold assets and interact with protocols like Ethena. WhileperformCallispayable, the contract lacks areceive()orfallback()function. Any direct ETH transfer (e.g., from a vault, a rewards distributor, or theowner) will revert.Impact
The contract cannot function as a long-term treasury or recipient for native ETH rewards. Any integration attempting to "push" ETH to this address will fail.
Recommendation
Add a
receive()function to allow the contract to accept native currency./// @dev Enable the contract to receive ETH.receive() external payable {}Kiln
Fixed in 197626f
Spearbit
Fixed by implementing the reviewer's recommendation.
Missing State Transition Validation in _setState()
Severity
- Severity: Informational
Submitted by
Optimum
Description
The
_setState()function currently updates the query state in storage without verifying if the transition from the current state tonewStateis legally defined. Since this utility is the central entry point for all state changes, it lacks a critical post-condition check to ensure the state machine's integrity.Impact
A buggy facet or subclass could accidentally trigger an illegal transition (e.g., moving a query from a terminal
REJECTEDstate back toACTIVE). Without a centralized check, these logic errors can propagate silently, leading to inconsistent contract states that are difficult to debug or recover.Recommendation
Implement the existing transition table to encode valid
(from -> to)pairs directly within_setState(). Reverting with a customInvalidTransition(State from, State to)error ensures that all state changes are validated regardless of which internal utility or facet initiates them.Kiln
Fixed in 5ad50ea8.
Spearbit
Fixed by implementing the reviewer's recommendation.
Functions renamings
Severity
- Severity: Informational
Submitted by
Optimum
Description
Rename the following functions to better reflect their actual behavior, specifically regarding fee accounting and toggle logic.
-
Rename:
previewApplyFees→previewApplyDepositRedeemFees -
Rename:
_previewApplyFees→_previewApplyDepositRedeemFees -
Rationale: Disambiguates between transaction fees and ongoing management fees.
-
Rename:
_handleFeesAfterOperation→_updatePostFeeCheckpoints -
Rationale: The current name is misleading; the function captures state snapshots rather than executing fee logic.
-
Rename:
convertToAssets()→previewAssetsWithFees() -
Rationale: Clarifies that the returned value is the amount remaining with fees.
-
Rename:
BaseVehicle.allow()→BaseVehicle.setAccessStatus() -
Rationale: "Allow" implies a one-way action; the new name reflects that it toggles both allow/disallow states.
-
Rename:
BaseVehicle._handleFeesBeforeOperation()→handlePerformanceAndManagementFees() -
Rationale: Replaces a generic lifecycle name with the specific types of fees being processed.
-
Rename:
Asset.assetsMatching(Asset[] memory queryOutput, Asset[] memory currentOutput): rename parameters as it is being used for input as well. -
Rename: In every Vehicle (and Facet)
_create_deposit→_createDeposit,_create_redeem→_createRedeem. Follow the existing camel case style for the function's name. -
Rename:
SingleAssetBaseVehicle._preview_deposit→_previewDeposit,SingleAssetBaseVehicle._preview_redeem→_previewRedeem. Follow the existing camel case style for the function's name -
Rename:
BaseVehicleChecks._onlyState→_onlyQueryState. The auth check is not about the contract's state but the query one. -
Rename:
BaseVehicleChecks._onlyOwner→_onlyQueryOwner. The auth check is not about the contract's state but the query one.
Kiln
Fixed in cdeb706.
Spearbit
Fixed by implementing the reviewer's recommendation.
Resiliency Against Non-Standard ERC-4626 Vault Withdrawal Fees
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Optimum
Description
Some erc4626 vaults might deduct fees during the
erc4626Vault.withdraw()call, returning fewer assets than requested, although the standard defineswithdrawas the following: "Burns shares from owner and sends exactly assets of underlying tokens to receiver."The current logic assumes the amount received exactly equals
_assetsToWithdraw. This discrepancy will causeunlock()to revert when it encounters a balance mismatch.Recommendation
Compute
_assetsToWithdrawbased on the difference of the balances of the calling contract before and after the call towithdrawto ensure accurate accounting.Kiln
We acknowledge this issue.
Spearbit
Acknowledged.
Repository-Wide Code Cleanup and Logic Consolidation
State
Severity
- Severity: Informational
Submitted by
Optimum
Description
- BaseVehicle.sol: L426:
else ifshould be used instead ofif. - BaseVehicle.sol: L232:
_assetsvariable is not used. - ERC4626Vehicle.sol: L84: The vehicle has facets but does not implement the create facet.
- ERC4626VehicleFacets.sol: L70:
__BaseVehicle_init()should be called ininitialize()instead. - BaseVehicleInitialization.sol: L137: redundant initialization of
$enabled. - ERC4626VehicleFactory.sol: L164: redundant check that already occurs in
_paramsChecks(). - ERC4626VehicleFactory.sol: L168: redundant check that already occurs in
craftInitialDeposit().
Kiln
Fixed in a558b015
Spearbit
Fixed by implementing the reviewer's recommendation.
Consider using OZ's Ownable2Step in the Account contract
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
Accountcontract is implementing a basic ownership mechanism that does not offer all the utility (migration, auth checks, ...) and security features already implemented in the battletestedOwnable2Stepcontract.Recommendation
Kiln should consider removing the current
$ownerlogic and replacing it by directly inheriting from theOwnable2Stepcontract.Kiln
We ACK this issue. Using OZ.Ownable2Step introduced more code and required guardrails (overriding renounceOwnership to prevent soft locks for ex.) and since this contract will only be used by other contract they would not need nor be able to transfer ownership of the Account contract.
Spearbit
Acknowledged.
Consider reverting if the setter/updater function does not perform any state change
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The current codebase is adopting the best practice to revert if a setter/updater function does not actually change the contract's state. This best practice is not always applied to all the functions.
Here are some examples:
Recommendation
Kiln should consider to apply the above best practice across all the contracts to be coherent with the adopted strategy.
Kiln
Fixed by commit
d3cad61ed1dc62bf571379478b0ee366a4602d9f. Additional fixes here commit864d1c142f707ade12918de3b1e43439d90a4f2dSpearbit
Fixed.
Sanity Checks
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
- ExternalAccessControl._grantRole:
_grantRoleallows granting therolerole toaddress(0). By implementing the sanity check at this point you can remove the same check fromAccessControlFactory._paramsChecks - CoreFactory.sol?lines=46,46: Add the sanity check
CheckLib.checkContract(createX, ...);for thecreateXinput parameter in theCoreFactoryconstructor. - AaveV3VehicleFacets.sol?lines=82,82: Consider reverting the Aave V3 vehicle initialization if the underlying Pool is not fully functional. Revert if
isActive == false || isPaused == true || isFrozen == true - BaseVehicleInitialization.sol?lines=118,118: consider reverting the
BaseVehicleInitialization.__BaseVehicle_initlogic if theFeeManagerhas not been fully initializing. The function could fetch the$.currentFeesConfigIdof theFeeManagerby calling thefee()function and revert if$.currentFeesConfigId == bytes32(0). - CompoundV3VehicleFacets.sol?lines=62,62: Consider reverting the Compound V3 vehicle initialization if the underlying market is not fully functional. Revert if
isSupplyPaused == true || isWithdrawPaused == true
Recommendation
Kiln should consider implementing the above sanity checks suggested to improve the security and avoid misconfigurations.
Kiln
Fixed by commit
1be5700fe9d515723c7cbe9eb253ee5c83826afb.Spearbit
Fixed. The suggested revert during the execution of
BaseVehicleInitialization.__BaseVehicle_initwhen the Fee Manager has not been initialized yet has been implemented in the commite1d58cee3f90fb81f4535e16b2649679d1de3e6b.Bulk Informational Issues
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
- ExternalAccessControl.sol?lines=281,281: The
CannotModifyPublicRoleerror inExternalAccessControl._notPublicScopedRolename is incorrect. The error name and meaning are specific to the action where these modifiers/functions are called on (to notify a public role) but the function is more general as a guard that "denies" access to public roles to a flow. Consider one of these two options: Option 1) just return abool(is public or not) and revert in the specific "root" function (where the meaning of the function is expressed). Option 2) use a more general-purpose error likePublicRoleAuthDeniedor something similar. - FreezablePausableBeacon.sol?lines=129,131: the
paused()sanity check inFreezablePausableBeacon.implementationimplementation can be removed. The same check is already performed by thewhenNotPausedmodifier. - AccessControlFactory.sol?lines=96,98: In
AccessControlFactory.spawnconsider replacing the direct call toCORE_FACTORY.deployContractwithFactoryLib.deployContractto be coherent with the rest of the best practice already adopted. Note: the same suggestion can be applied to the OOS code (in the Phase 0 context) inMultiVehicleFactory._subSpawn - MorphoBlueVehicleFactory.sol?lines=129,129: rename the input parameter
morphoImplementationin theMorphoBlueVehicleFactoryconstructor tomorphoBlueVehicleImplementationto avoid confusing it with the "real" Morpho Blue (market). - CoreFactory.sol?lines=157,157: Rename the named return parameter
vehiclefor theCoreFactory.cloneContractfunction tocontract_ - Vehicle.sol?lines=89,89 + Vehicle.sol?lines=108,108: Consider refactoring the
convertToAssetsandconvertToSharesfunctions inVehicleto return the wholeAssettype instead of the raw value to offer a better DX - BaseVehicleEvents.sol?lines=46,60: consider declaring the
addresstype input of theDeployerInitialized,FeeManagerInitialized,ModulesManagerInitializedandAccessControlInitializedevents asindexed - BaseVehicleInitialization.sol?lines=131,135: consider emitting a specific event when the
$.forbiddenAddressesare configured during theBaseVehicleInitialization.__BaseVehicle_initexecution - BaseVehicleQueryStateSetter.sol?lines=97,102: the transition to the
REJECTEDstate should happen only via the execution of theBaseVehicleQueryStateSetter._rejectfunction. Look in the whole codebase where_transition(queryId, State.REJECTED, query.input);is used and replace it with the_rejectcall. - BaseVehicle.sol?lines=756,757: consider swapping the order of the
ignorePayoutFeesandignoreOngoingFeesinput parameters of theBaseVehicle._estimatefunction. The "ongoing fee" is applied before the "payout one". - FeeManager.sol?lines=265,270: consider emitting a specific event when
FeeManager.onUpdateis executed and$.cache[msg.sender].applicableConfigId != $.currentFeesConfigIdto signal that the new fee config has been applied to the calling vehicle. - MorphoBlueVehicle.sol?lines=181,192:
assets[0].asset = address(this);is repeated in both else/if brackets and can be in the main body of the function. - FeeManager.sol?lines=159,159:
NoRecipientsis not used. - Consider refactoring every
Math.mulDivcall to use explicit rounding directions. Some of the existingMath.mulDivcalls do not specify it.
Recommendation
Kiln should consider implementing the suggestions listed above.
Kiln
Fixed by commit
6a2265d0a75a081253f67e0beb1f68661b4b431b.Spearbit
Kiln has also provided the following additional commits: commit
b329803238bd0e77c5232f3eb24d06a0e156fe89.Fixed. The remaining points have been acknowledged by Kiln.
forbiddenAddresses configured by the factories could be seen as meaningless
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
forbiddenAddressesconfigured for a base vehicle is used to prevent the creation of a query that has theowneror thereceiver(of the Query) part of these forbidden lists.The Factories of these vehicles are pre-configuring the
forbiddenAddresseswith some specific addresses. Let's take a look at theAaveV3VehicleFactoryto make an example:address[] memory _forbiddenAddresses = new address[](1); _forbiddenAddresses[0] = params.poolAddressesProvider;While we can argue that indeed the Pool Address Provider address is an "invalid" address to be configured as the query
ownerorreceiver, we could also argue that also the underlying'sAToken,VToken,Pool(and so on) are invalidowner/receiverof a Query. The list could go on and on.The
forbiddenAddressesshould only be used to configure specific addresses that could break the Vehicle's logic or enable security exploits.The current usage, like the one done by the
AaveV3VehicleFactory(and all the other existing factories) could be seen as meaningless and limited given that the addresses to be configured with are infinite.Recommendation
Kiln should, for each existing Vehicle Factories, look if there are specific addresses to be configured as "forbidden". Only addresses that could break the vehicle or create security exploits should be configured as forbidden.
Kiln
Fixed by commit
52aabac0518d9d9afd4a9c7621aaf02ac01e2f04. Additional changes have been made in the commit48b39a1c009007b6d1f04153c6589eeab08ebe0f.Spearbit
Fixed.
spawn function can be improved for some of the existing factories
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
spawnfunction of theFeeManagerFactory,ModulesManagerFactory,AccessControlFactory,AccountListFactoryandOwnerRegistryFactoryfactories can be refactored and improved.The below suggestion reference the
FeeManagerFactorycode but it can be applied to all the above listed factories:- take
_feeManagerAddressfrom theFactoryLib.deployContractand not fromthis.getDeploymentAddress - declare
getDeploymentAddressasexternaland notpublic(it's not needed public anymore) - move the
SpawnedFeeManagerevent to the bottom of the function's logic (like the in the Vehicle factories)
Recommendation
Kiln should consider to implement the above suggestions.
Kiln
Fixed by commit
102df6af2ce5f5a09191e7100bba394e70dd2c1d.Spearbit
Fixed.
Consider updating the STEAM standard to require the Query to provide a non-empty Asset[] output array
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The current
STEAMstandard and the corresponding code implementation allow the query creator to specify an empty array for thequery.outputattribute.This choice create inconsistencies and less strict code behaviors that should be avoided. For a standard it's always a good design choice to require being explicit and avoid any possible confusion or misconfiguration.
If the user or caller does not care about the query results and accepts any possible outcome from a
DEPOSITorREDEEMoperation, it should explicitly express it as a valid, non-emptyquery.output.Let's make a practical example: the user wants to deposit
USDCin theAaveV3Vehicleand accept any possible outcome from such an operation.Right now the user can choose to: specify an empty
query.outputor be explicit and set it to[{asset: address(vehicle), value: 0}].By being explicit we can ensure that the query has been correctly configured and the user has made no mistakes. This will become even more crucial when the protocol will manage more complex and exotic vehicles that could output multiple assets.
Recommendation
Kiln should consider changing the
STEAMstandard and consider an empty Queryoutputas an invalid state.After performing such a change in the standard, the changes should also be reflected in the code:
- The
Asset.assetsMatchingfunction should remove theif (_queryOutputLength == 0) { return true; }early return statement - The
SingleAssetBaseVehicle._validateOutputsfunction should revert withRejectionLib.rejectWithZeroInputValue()whenoutputs.length == 0
Kiln
Fixed by commit
55e570e6baa88d886d73e9ea984767fcf2c01884Spearbit
Fixed.
Use forceApprove instead of safeIncreaseAllowance in the Factory
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
In
Factory.craftInitialDepositthesafeIncreaseAllowanceis currently used- the Vehicle has just been deployed so we can assume that it won't have any existing allowance for
address(this) - we expect the Vehicle to pull the whole
params.initialDepositSizewhenBaseVehicle.createis executed to manage the initial deposit query
Recommendation
Kiln should replace
safeIncreaseAllowancewithforceApproveKiln
Fixed by commit
557f5eaSpearbit
Fixed.
Vehicle functions convertToAssets and convertToShares should be better documented
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
Vehicleis a utility library that is conceived to support multi-asset vehicles but theconvertToAssetsandconvertToSharesfunctions will revert if the vehicle is not a single-asset vehicle.Recommendation
Kiln should consider renaming and documenting those functions and be explicit on the usage.
Kiln
Fixed by commit
04b0b9d7f7dda617ebaf4ac9b7024e7962fac4abSpearbit
Fixed.
Remove the _recover function's implementation from all those vehicle that cannot transition to the RECOVERING state
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
AaveV3Vehicle,CompoundV3Vehicle,ERC4626Vehicle,MorphoBlueVehicleandWrapperVehiclecannot logically transact a query to theRECOVERINGphase. This means that everytime that someone calls the vehicle'srecover(Query calldata query)function, the vehicle will revert with theBaseVehicleErrors.InvalidStateresult when the_onlyStatefunction (triggered by thesteamOperation(query, State.RECOVERING)modifier) is executed.To avoid any confusion and possible future mistakes, all those vehicles should fully remove the
function _recover(Query calldata query, Id qid)implementation from their contract.As a general rule of thumb, if the state cannot be reached, the vehicle's should never implement the
internalcorresponding function that would manage the state.Recommendation
Kiln should remove the
_recoverfunction's implementation from all those vehicles that cannot transition to theRECOVERINGstate for a query.Kiln
Fixed by commit
4269470b84c8a95f7c4838eabdc3e550400360b7We removed the overrides + we also overriden
resume()andrecover()to immediately revert and reduce bytecode size (no Facet handling)Spearbit
Fixed.
Consider allowing integrator to fetch the fee detail snapshotted to the Query
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
When a Query has been created, the
BaseVehiclesnapshot a specific query configuration to be applied in the unlocking phase. At the moment there's not a way for the query's owner to retrieve that information properly.Kiln should offer these two functions:
- In
BaseVehiclea function that given aQuery qit fetches thefeesConfigIdsnapshotted to it and retrieves thedepositFeeBpsandredeemFeeBpsassociated to it in theFeeManager - In the
FeeManagera function that given abytes32 configIdit returns thedepositFeeBpsandredeemFeeBps
Recommendation
Kiln should consider improving the user's and integrator DX and allow external entities to fetch the fee configuration associated with a query.
Kiln
Fixed by commit
53abd3e97df3480356d1227dd46db558421220e7.Spearbit
Kiln has also provided the following additional commits: commit
b8b982fa266355b1494bd05779be377c784bad4fand commit3e680e46f128e2ff9359d99ddfa2af52c864bb7e.Now the config can be queried directly from the Vehicle itself and the FeeManager will revert if the specified
configIddoes not exist.Consider avoiding taking payout fees when the query has been generated by the FeeManager
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
StErMi
Description
When the user executes the
unlockflow to receive the vehicle's shares (from aDEPOSIToperation), theFeeManagerwill take an "operation fee" that will transfer part of the shares to theFeeManager.At some point the entity with the role
FEE_MANAGER_DISPATCH_ERC20will executeFeeManager.dispatchERC20and dispatch those shares to everyfeeRecipientsthat will need to execute aREDEEMoperation on the same vehicle to receive the underlying asset. To the generated query, theFeeManagerwill also take a payout fee that will distribute the fees (as underlying assets) to theFeeManageragain.Recommendation
Kiln should consider to avoid taking the "operation fee" on the redeem query generated to withdraw the fee distributed to the
feeRecipientsKiln
Acknowledged. According to our discussion, we finally decided to revert the changes and to come back to the original behavior, see the revert commit 7ae0f3b7.
Spearbit
Kiln will acknowledge the Finding. A solution has been proposed but it was reverted since the complexity of the implementation and the possible side effects were not worth the effort and the degradation of overall security and gas cost.
Some "open" functions allow anyone to generate spammy events
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
In the codebase there are functions that allow anyone to execute them and generate "spammy" events that need to be excluded by the Kiln monitoring and analytics system.
FeeManager.onOperations->OperationalFeesCollectedeventFeeManager.applyFees->TransactionalFeeCollectedeventKeeperLib.startJob->JobStartedeventKeeperLib.stopJob->JobDoneeventKeeperLib.cancelJob->JobCancelledevent
Recommendation
Kiln should be aware of all these "open" functions and that the events generated by those functions must be properly filtered by their monitoring and analytics tools.
Kiln
Fixed by commit
d2881fec31784f6bb85923dc8852537e00b81b27Spearbit
Fixed.
Vehicles do not follow a common style and best practice in their implementation
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
A lot of refactor has been already done and we can see big improvements but there are still these incoherences that should be fixed and refactored in the context of how each Vehicle has been implemented.
We can see for example some differences comparing the Morpho Vehicle with the Aave V3 Vehicle:
- In
AaveV3VehicleFacetsthe approval is executed on demand when the_create_depositis executed. InMorphoBlueVehicleFacetsinstead is performed during the initialization phase and for the infinite amount (type(uint256).max) AaveV3VehicleFacetsit's called_inputAsset.forceApprove(...)while in Morpho it is calledSafeERC20.forceApprove(IERC20($.asset), ...)- the code in
AaveV3VehicleFacetsis "dense" (no new line between instructions), whileMorphoBlueVehicleFacetsis instead sparse with new lines between instructions.
Recommendation
Kiln should push to create a common and standard style and best practice guide that should be strictly applied to every vehicle developed.
In the specific context of the above example, we suggest Kiln the following reccomendations:
- Avoid the "infinite approval" and implement, like in the Aave V3 vehicle, on-demand ad-hoc approval
- Adopt the
SafeERC20usage used in the Aave V3 vehicle - Adopt (and apply) in the other vehicles the style used in the Morpho Vehicle.
Kiln
Fixed by commit
bc84f3292727262be48411ec69af4394b48dfefc.Spearbit
Fixed. Note: the suggestion to avoid the infinite approval in the MorphoBlueVehicle was intentionally not applied. Kiln kept the infinite approval because the underlying Morpho Blue protocol is immutable, which the reviewer agreed with.
Inefficient O(n^2) Duplicate Check in _setRecipients
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Description
The
_setRecipientsfunction checks for duplicate recipient addresses using a nested loop, resulting inO(n^2)complexity. For each recipient, it compares the address with all subsequent recipients to detect duplicates. This approach is unnecessarily inefficient and does not scale well in case the count of addresses of the recipients grows large.Recommendation
Require the
newRecipientsarray to be sorted by address and replace the nested duplicate check with a single pass that verifiespreviousAddress < currentAddress. This reduces the validation complexity fromO(n^2)toO(n)and improves gas efficiency.Kiln
We ACK this one as this is not a common execution path (rarely called) so we prefer UX over efficiency here.
Spearbit
Acknowledged. The issue can be left unresolved as it is a gas optimization and does not actively threaten the protocol.
Redundant Admin Check in grantRole and revokeRole
State
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Description
The
grantRoleandrevokeRolefunctions applyonlyRole(getRoleAdmin(role))even though the same admin check is already enforced in the inherited OpenZeppelingrantRoleandrevokeRolethrough thesupercall chain. This results in redundant authorization checks and unnecessary gas overhead. This applies only tograntRoleandrevokeRole, while the check must remain ingrantScopedRoleandrevokeScopedRolesince they implement custom scoped-role logic.Recommendation
Remove the redundant
onlyRole(getRoleAdmin(role))modifier fromgrantRoleandrevokeRole, since the inherited OpenZeppelin implementation already enforces the same admin check. Keep the modifier ongrantScopedRoleandrevokeScopedRole.Kiln
Fixed by https://github.com/kilnfi/railnet/pull/375/changes/0b490ad609212a1a12b69cf7e357deedb5b4945f
Spearbit
Verified Fix,
grantRoleandrevokeRolenow check theonlyRole(getRoleAdmin(role))only through thesupercall chain.CompoundV3Vehicle and MorphoBlueVehicle Cannot Claim Incentive Rewards
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Description
The
CompoundV3VehicleandMorphoBlueVehicleintegrations currently do not provide a mechanism to claim incentive rewards generated by their positions.Both Comet and Morpho Blue distribute rewards separately from the base lending yield, and these rewards must be explicitly claimed through their respective reward distribution contracts. Since the vehicles do not expose any functionality to trigger reward claims, incentives accrued by the vehicles remain unclaimed.
- Compound rewards documentation: https://docs.compound.finance/
- Morpho rewards documentation, new rewards via Merkl: https://docs.morpho.org/learn/concepts/rewards/
As a result, any rewards accumulated by these vehicles are not collected or redistributed within the system until a module is added.
Recommendation
Consider implementing a rewards claiming module for
CompoundV3VehicleandMorphoBlueVehicle.Kiln
We ACK this issue
- We do not plan yet on supporting CompoundV3Vehicle additional rewards via a module
- The Interceptor pattern in place helps us support Merkl natively on all our contracts
Spearbit
Acknowledged. Not fixing this issue will result in the Vehicle not being able to redeem compound rewards for the time-being and the protocol has accepted the risk. A module can be added in the future that enables receiving the rewards. Note: the CompoundV3Vehicle has since been fully removed from the codebase in later commits.
CoreFactory Mishandles ETH Forwarding and Refund Accounting Across CreateX and Clone Flows
State
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Description
CoreFactorydoes not consistently handlemsg.valueand ETH refunds across its deployment paths. This can lead to ETH being unintentionally trapped inside the factory and later used in ways that are not clearly tied to the caller’s transaction.Several situations illustrate this behavior:
-
ETH can be trapped when
data.length == 0: IndeployVehicleanddeployContract, the branchif (data.length == 0)callsCREATEX.deployCreate2(salt, initCode)without forwarding ETH. Ifmsg.value > 0, the ETH remains inCoreFactoryinstead of being used for deployment or refunded to the caller. -
Refunds from
CreateXaccumulate in the factory: In initialization flows usingdeployCreate2AndInit,CreateXsends any excess ETH to the providedrefundAddress, which in this integration resolves toCoreFactory. The factory accepts these refunds viareceive()but does not forward them to the original caller, allowing ETH to accumulate in the contract. -
Refunds may not correspond to the current deployment:
CreateXrefunds its entire balance, which may include ETH previously forced into the contract (e.g., viaSELFDESTRUCT). This means the refund amount is not necessarily attributable only to the current transaction. -
Factory balance can later be used during clone initialization: In
cloneVehicleandcloneContract, initialization calls forwardvalues.initCallAmountrather than bounding the ETH tomsg.value. If the factory already holds ETH, callers may trigger initialization calls that spend funds previously accumulated in the factory. -
Constructor value assumptions: If
values.constructorAmount > 0, the constructor of the providedinitCodemust be payable; otherwise deployment will revert.
Recommendation
Standardize ETH handling across all deployment paths:
- Revert when
data.length == 0 && msg.value > 0, or ensure ETH is forwarded to the deployment call. - Avoid retaining refunds from
CreateXinsideCoreFactory. Refund excess ETH directly to the caller or explicitly forward it after deployment. - Ensure initialization calls cannot spend ETH already held by the factory. The ETH forwarded during initialization should be bounded by the current
msg.value. - Validate or clearly document that deployments using a nonzero
constructorAmountrequire a payable constructor.
Kiln
Fixed in commit db83c21
Spearbit
Fixes verified.
Created event emission does not respect the STEAM specification
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
STEAMspec for theCreatedevent states:MUST be emitted before any state transitions occur for the query.
The current
BaseVehicle.createimplementation does not follow the spec definition by emitting the event at the very end of the flow after that the internal state of the contract has been updated by the execution of_createRecommendation
Kiln should perform the following change to the code for the
BaseVehicle.createfunction:+emit Created(_qid, query);(newState, _possibleNext, _assets) = _create(query, _qid, __totalSupply, __totalAssets); if (_handleFees) { _handleFeesAfterOperation();} -emit Created(_qid, query);Kiln
Fixed by commit
3b3a631ac66439f9c1551b2fb01b643b9be269a1Spearbit
Fixed.
STEAM standard fee's lifecycle section is outdated
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The "Fee Application During Query Lifecycle" section of the
STEAMstandard states- Fees are only applied when transitioning from
EMPTYtoPROCESSINGorUNLOCKINGstates viacreate(). - Fees are not applied during recovery operations (
recover()method), allowing users to reclaim assets without additional charges.
But vehicles do not strictly apply these rules. A vehicle implementation can override the
_shouldHandleFeesfunction and apply the fees not only when the query is in theEMPTYstate but also when it's in theUNLOCKINGorRECOVERINGstate.function _shouldHandleFees(Mode, State queryState) internal pure virtual returns (bool) { return queryState == State.EMPTY; }Recommendation
Kiln should update the
STEAMstandard to correctly define all those cases when the "Ongoing Fees" and "Operational Fees" should be applied.Kiln
Fixed by commit
8e26ac51bdee8dfd43d7c1a9204e474db09cb3c5.Spearbit
Fixed.
Improve the totalAssets() definition in the STEAM standard
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The
STEAMstandard for thetotalAssets()definition states:- MUST return a
uint256representing the total amount of assets (both in-contract and invested).
The current implementation, at least for the Vehicles that have been reviewed in the current Phase, does not include the underlying
_asset()balance that has been donated directly to the vehicle itself.Recommendation
Kiln should update the
STEAMstandard definition for thetotalAssets()to specify that underlying donations directly to the Vehicle won't be included in the value returned by the function itself.Kiln
Fixed by commit
21b5a98b263ac45e830f101e65926b0a37250ec3Spearbit
Fixed.
Consider refactoring the ModulesManager module management/allowance logic
State
Severity
- Severity: Informational
Submitted by
StErMi
Description
The current implementation of the
ModulesManagerworks like this:A module (a "feature") is identified by a
bytes32 idand an implementation contract (of such feature)address moduleTheMODULE_MANAGERrole can- Add a module via
addModule(bytes32 id, address module) - Update a module via
updateModule(bytes32 id, address module) - Remove a module via
removeModule(bytes32 id) - Approve a "pending module" via
approvePendingModule(bytes32 id) - Cancel a "pending module" via
cancelPendingModule(bytes32 id)
The
addModuleandupdateModuledo not instantly add/update the module to the new address but they will create a "pending module request" that needs to pass a timelock period and then be finalized via theapprovePendingModuleaction.On the other side the Vehicle will allow/disallow the execution of a module via the
allowModule(bytes32 id)anddisallowModule(bytes32 id)functions.The Vehicles are allowing/disallowing the "feature" (the
bytes32 id) and not the specific implementation of the feature which is instead identified by the(bytes32 id, address module)tuple.Let's assume that "Feature A" has been added by the Module Manager and has been finalized with the tuple
(featureA_id, featureA_address)and let's assume theVehicle1has approved it.What should happen in these scenarios?
- The feature is removed via
removeModule(featureA_id), is "replaced" viaaddModule(featureA_id, featureB_address)and finalized viaapprovePendingModule(featureA_id). TheVehicle1has "blindly" allowed the "feature" which now has changed implementation. - The feature implementation is updated via
updateModule(featureA_id, featureB_address)and finalized viaapprovePendingModule(featureA_id). TheVehicle1has "blindly" allowed the "feature" which now has changed implementation. - The feature implementation is migrated to a new implementation
featureA_new_addressbut the update is still pending approval. Should theVehicle1be able to still executefeatureB_addressimplementation even if it is being replaced for some specific reason? Or if there's a "pending" approval the execution should be blocked?
Recommendation
Assuming that the
ModulesManageris not unique to each Vehicle, Kiln should consider refactoring and improving theModulesManagerto offer better management and security for the Vehicle that will execute in a delegatecall context those "module implementations".The Vehicle should be able to approve the final implementation of a "feature" that will be executed, which is identified by the
(moduleId, moduleImplementationAddess).Kiln
Fixed by commit
278c93f7b9e7f1e91a70ab7e18e5bc30e68f17c1.Spearbit
Fixed. The
BaseVehicle.execfunction now tracks the execution of the external module via theBaseVehicleEvents.ModuleExecutedevent, the additional suggestion has been implemented in the commit7bff310be492403ad0dcbfdcf3bb446ba1a5ba29Overriding _shouldHandleFees function in future implementations can have unexpected impacts
State
Severity
- Severity: Informational
Submitted by
zigtur
Description
The
_shouldHandleFeesfunction is marked as virtual. In future implementations, it may get overridden.However, such implementation will most probably have to review the
ignoreOngoingFeesparameter used during the call to_estimate.In the current
createfunction, the ongoing fees are ignored because_handleFeesis true and fees were already minted in_handleFeesBeforeOperation. This is shown in the_estimatecall, it ignores these fees.function create(Query calldata query) // ... { // ... bool _handleFees = _shouldHandleFees(query.mode, State.EMPTY); if (_handleFees) { __totalSupply = _handleFeesBeforeOperation(__totalSupply, __totalAssets, _qid); } // ... // Check if the required output matches the estimated output { (Asset[] memory _output, bytes32 _appliedFeesConfigId) = _estimate(query.input, query.mode, EstimationType.OUTPUT, false, true, __totalSupply, __totalAssets); BaseVehicleStore.getStorage().queries[_qid].feesConfigId = _appliedFeesConfigId; AssemblyLib.revertIfBytes(_validateConstraints(query, _output)); }Recommendation
Overriding
_shouldHandleFeesis likely to require overriding the BaseVehiclecreatefunction too. This should be documented somewhere.Kiln
Fixed in commit 231c593 by using
_shouldHandleFeesvalue to ignore the ongoing fees increate.Spearbit
Fixed. The fix bounds two behaviors together: "fees are being taken" and "fees are excluded from estimation".
This fixes the issue. However, it is a potential edge-case for future vehicles.
Scope and code overview
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Alireza Arjmand
Scope
.├── abstracts│ ├── ExternalRBAC.sol│ ├── ExternalRBACUpgradeable.sol│ ├── FactoryBase.sol│ └── Interceptor.sol├── common│ ├── Account.sol│ ├── ExternalAccessControl.sol│ ├── FreezablePausableBeacon.sol│ └── interfaces│ ├── IExternalAccessControl.sol│ └── IFreezablePausableBeacon.sol├── docs│ └── STEAM.md├── factories│ ├── common│ │ └── AccessControlFactory.sol│ ├── CoreFactory.sol│ ├── interfaces│ │ └── ICoreFactory.sol│ └── vehicles│ ├── AaveV3VehicleFactory.sol│ ├── CompoundV3VehicleFactory.sol│ ├── ERC4626VehicleFactory.sol│ ├── FeeManagerFactory.sol│ ├── ModulesManagerFactory.sol│ └── MorphoBlueVehicleFactory.sol├── INDEX.md├── libs│ ├── AccessControl.sol│ ├── Assembly.sol│ ├── Asset.sol│ ├── Check.sol│ ├── Constant.sol│ ├── Error.sol│ ├── Factory.sol│ ├── Keeper.sol│ ├── Query.sol│ ├── Rejection.sol│ ├── Roles.sol│ ├── Shares.sol│ └── Vehicle.sol├── steam│ ├── IVehicle.sol│ └── Query.sol└── vehicles ├── aave_v3 │ ├── AaveV3Vehicle.sol │ ├── abstracts │ │ ├── AaveV3VehicleErrors.sol │ │ └── AaveV3VehicleEvents.sol │ ├── facets │ │ └── AaveV3VehicleFacets.sol │ ├── interfaces │ │ ├── IPool.sol │ │ ├── IPoolAddressesProvider.sol │ │ ├── IPoolDataProvider.sol │ │ └── IScaledBalanceToken.sol │ ├── libs │ │ ├── AaveV3VehicleStore.sol │ │ └── AaveV3VehicleStructs.sol │ ├── modules │ │ └── AaveV3Merkl.mod.sol │ └── VEHICLE.md ├── base │ ├── abstracts │ │ ├── BaseVehicleAssetUtils.sol │ │ ├── BaseVehicleChecks.sol │ │ ├── BaseVehicleErrors.sol │ │ ├── BaseVehicleEvents.sol │ │ ├── BaseVehicleInitialization.sol │ │ ├── BaseVehicleQueryStateGetter.sol │ │ └── BaseVehicleQueryStateSetter.sol │ ├── BaseVehicle.sol │ ├── FeeManager.sol │ ├── interfaces │ │ ├── facets │ │ │ ├── ICreateFacet.sol │ │ │ ├── IInitializeFacet.sol │ │ │ ├── IRecoverFacet.sol │ │ │ ├── IResumeFacet.sol │ │ │ └── IUnlockFacet.sol │ │ ├── IBaseVehicle.sol │ │ ├── IDistributionCreator.sol │ │ ├── IFeeManager.sol │ │ ├── IModule.sol │ │ ├── IModulesManager.sol │ │ └── IModuleTarget.sol │ ├── libs │ │ ├── BaseVehicleConstants.sol │ │ └── BaseVehicleStore.sol │ ├── ModulesManager.sol │ └── SingleAssetBaseVehicle.sol ├── compound_v3 │ ├── abstracts │ │ ├── CompoundV3VehicleErrors.sol │ │ └── CompoundV3VehicleEvents.sol │ ├── CompoundV3Vehicle.sol │ ├── facets │ │ └── CompoundV3VehicleFacets.sol │ ├── interfaces │ │ └── IComet.sol │ ├── libs │ │ ├── CompoundV3VehicleStore.sol │ │ └── CompoundV3VehicleStructs.sol │ └── VEHICLE.md ├── erc4626 │ ├── abstracts │ │ ├── ERC4626VehicleErrors.sol │ │ └── ERC4626VehicleEvents.sol │ ├── ERC4626Vehicle.sol │ ├── facets │ │ └── ERC4626VehicleFacets.sol │ ├── libs │ │ ├── ERC4626VehicleStore.sol │ │ └── ERC4626VehicleStructs.sol │ └── VEHICLE.md └── morpho_blue ├── abstracts │ ├── MorphoBlueVehicleErrors.sol │ └── MorphoBlueVehicleEvents.sol ├── facets │ └── MorphoBlueVehicleFacets.sol ├── libs │ ├── MorphoBlueVehicleStore.sol │ └── MorphoBlueVehicleStructs.sol ├── MorphoBlueVehicle.sol └── VEHICLE.mdCodebase Overview
Railnet is Kiln's vault platform built around STEAM, an in-house standard for vault operations that supports both synchronous and asynchronous flows. Every operation is a query — a struct describing the owner, receiver, input/output assets, and mode (deposit or redeem) — that moves through a fixed state machine (
EMPTY → PROCESSING → PAUSED / UNLOCKING / RECOVERING / REJECTED → SETTLED) via thecreate,resume,unlock, andrecoverentrypoints. Synchronous vehicles settle a query in two calls; asynchronous ones park it until the underlying protocol settles.The core of the phase is the BaseVehicle framework: an upgradeable ERC-20 vault whose shares are priced against the assets a concrete vehicle holds in its yield source. It uses namespaced (ERC-7201) storage throughout, delegates lifecycle logic to per-vehicle facet contracts to stay under the bytecode limit, and validates queries through a rejection model in which invalid queries become recoverable rather than reverting.
SingleAssetBaseVehiclespecializes the framework for the common single-asset-in/shares-out shape. Two satellite contracts extend a vehicle's behavior: theFeeManager(management, performance, deposit, and redeem fees, with per-query fee-config snapshots) and theModulesManager(timelocked registration of modules that vehicles execute via delegatecall).Surrounding infrastructure includes a shared role-based access control system (
ExternalAccessControl, supporting global, scoped, and public roles), beacon-proxy upgradeability with pause and permanent-freeze controls (FreezablePausableBeacon), and a factory stack:CoreFactoryperforms deterministic deployments via CreateX, and per-component factories spawn beacon, proxy, and an initial burned deposit that protects new vaults against share-inflation attacks.Four synchronous yield-source vehicles are in scope, each following the same layout (main contract, facets, store library): Aave v3 (supplies to the Aave pool), Compound v3 (supplies to a Comet market, was fully removed in later commits), ERC-4626 (deposits into an external vault), and Morpho Blue (supplies to a Morpho market). A set of small shared libraries (asset/query handling, share conversion, roles, errors, keeper events) completes the scope.