feat(utils): MoolahVaultAccount — principal-baselined account for the protocol lisUSD vault position - #229
Conversation
Pull Request ReviewThis Solidity PR adds a new UUPS-upgradeable MoolahVaultAccount that tracks a fixed lisUSD principal baseline, permits role-gated principal management and emergency exits, and allows a bot to distribute only surplus yield to manager-approved recipients. It also adds atomic deployment scripts, extensive unit and BSC fork tests, a mock ERC-4626 vault, and updates CI to run the new fork test separately. Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
Pull Request ReviewThis PR adds a new UUPS-upgradeable Solidity account for custodying the protocol’s lisUSD MoolahVault position, tracking a fixed principal baseline, harvesting surplus yield to manager-approved recipients, and supporting role-gated principal and emergency withdrawals. It also adds atomic deployment scripts, extensive unit and BSC fork tests with a mock yield vault, and updates CI to run the new fork test in the fork-test job. Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
… protocol lisUSD vault position Holds the protocol's own lisUSD MoolahVault position behind an explicit `principal` baseline so a bot can harvest only the surplus above it to whitelisted destinations. Today the position sits directly in the B0c6 multisig: every yield sweep is a manual multisig transaction and nothing on-chain separates corpus from yield. Accounting: `totalAssets()` reads `vault.convertToAssets(vault.balanceOf(this))` so accrued-but-unminted fee shares net out; `claimableYield()` is `max(0, totalAssets() - principal)` and clamps at 0 because vault NAV is not monotonic. `principal` never floats with NAV — it moves only through permissioned calls. Roles: DEFAULT_ADMIN_ROLE (protocol TimeLock) holds the upgrade key, `setPrincipalOwner` and role administration; MANAGER (B0c6) moves principal, corrects the baseline, owns the recipient whitelist and can force the position out; BOT calls `claimYield` only; PAUSER calls `pause` only. No exit lets its caller name a destination. `emergencyWithdraw` transfers the vault shares themselves to `principalOwner` rather than redeeming them, so it never walks the withdraw queue and is not bounded by vault liquidity — `withdrawPrincipal` and `claimYield` can both revert NotEnoughLiquidity() while it still succeeds. Deployment is atomic: the `initialize` calldata rides in the `ERC1967Proxy` constructor, the implementation burns its initializer, roles go straight to their final holders, and the deployer never holds a role. That closes the window whose absence the 2026-05-25 lisAster proxy hijack depended on. Tests: 72 unit tests against a mock vault with liquidity, whitelist, NAV and withdraw-shortfall levers; 8 fork tests against the live lisUSD vault at block 116,433,631 asserting exact selectors. Both deploy scripts simulate on BSC mainnet and pass their own post-broadcast self-checks.
e48a382 to
a3390ee
Compare
Pull Request ReviewThis Solidity PR adds a new UUPS-upgradeable Sensitive ContentNo sensitive content detected. Security Issues🟠 [HIGH] Direct share top-ups can be harvested as yield before the principal baseline is updated
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
Four fixes from the PR #229 audit. 1. `withdrawPrincipal` is now `whenNotPaused`. A halted contract must not let principal leave piecemeal: while the pause holds, the only sanctioned exit is `emergencyWithdraw`, which takes the whole position back to `principalOwner` in one move and cannot be walked down amount by amount. `emergencyWithdraw` stays outside the pause — the incident that pauses this contract is the reason to call it — and MANAGER holds `unpause`, so this is a speed bump for MANAGER rather than a trap. 2. MANAGER is now the role admin of PAUSER as well as BOT. PAUSER on BSC is a 1-of-15 Safe, so any one of fifteen keys can halt `claimYield` and `depositPrincipal`; revoking a key that is holding the contract down must not wait on a 24h TimeLock proposal. The deploy script asserts both role-admin edges in its post-deploy self-check. 3. `increasePrincipal`'s NatSpec described the launch wrongly. The baseline comes from `initialize`, and at launch B0c6 only transfers its shares in — calling `increasePrincipal` on top of that would count the same corpus twice, and the baseline cannot be lowered again, so `claimableYield()` would sit at 0 for good. The doc now states that and names the two sanctioned ways to move the baseline afterwards: `depositPrincipal` / `withdrawPrincipal`, where the baseline follows the funds in the same call; or a share transfer paired with `increasePrincipal` in ONE transaction, where the MultiSend requirement applies. 4. The implementation deploy script's initializer-lock check was vacuous. It passes the deployer EOA as `_vault`, so `initialize` reverts at `IMoolahVault(_vault).asset()` whether or not the initializer was burned — `require(!initializable)` would have cleared an implementation with no `_disableInitializers()`. It now also asserts the revert data is `Initializable.InvalidInitialization`, which can only come from the burned initializer, before any argument is touched. Tests: 74 unit (2 new, 1 inverted for the pause semantics) and 8 fork tests pass; both deploy scripts simulate on BSC mainnet and pass their self-checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-followup fix(utils): MoolahVaultAccount audit follow-up
Pull Request ReviewThis Solidity/Foundry PR adds a new UUPS-upgradeable Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
2 participants
Summary
Adds
MoolahVaultAccount, an upgradeable account contract that holds the protocol's own lisUSDMoolahVaultposition behind an explicitprincipalbaseline, so a bot can harvest only the surplus above that baseline to whitelisted destinations. Today the position sits directly in the B0c6 multisig, which means every yield sweep is a manual multisig transaction and there is no on-chain record of what part of the position is corpus and what part is yield.Change type
Contracts changed
MoolahVaultAccountsrc/utils/MoolahVaultAccount.solDeployMoolahVaultAccountscript/utils/deploy_moolahVaultAccount.s.solDeployMoolahVaultAccountImplscript/utils/deploy_moolahVaultAccount_impl.s.solMockYieldVaulttest/utils/mocks/MockYieldVault.solMoolahVaultAccountTesttest/utils/MoolahVaultAccount.t.solMoolahVaultAccountForkTesttest/utils/MoolahVaultAccountFork.t.solNo existing contract is modified. The only non-Solidity change is
.github/workflows/unit-tests.yaml, which addsMoolahVaultAccountForkTestto both the--no-match-contractlist of the unit job and the--match-contractlist of the fork job.Interface changes
All new — this is a new contract.
Roles
DEFAULT_ADMIN_ROLE_authorizeUpgrade,setPrincipalOwner, MANAGER administrationMANAGERunpause, BOT/PAUSER administrationBOTclaimYieldonlyPAUSER0xEEfe…5Bd8, 1-of-15pauseonlyExternal / public functions
initialize(address admin, address manager, address bot, address pauser, address vault, address principalOwner, uint256 principal, address[] yieldRecipients)totalAssets() view returns (uint256)—vault.convertToAssets(vault.balanceOf(this))claimableYield() view returns (uint256)—max(0, totalAssets() - principal)previewClaim() view returns (uint256 claimable, uint256 withdrawable, uint256 claimableNow)depositPrincipal(uint256 assets)—MANAGER,nonReentrant,whenNotPausedwithdrawPrincipal(uint256 assets)—MANAGER,nonReentrant,whenNotPausedincreasePrincipal(uint256 assets)—MANAGER; raises the baseline when the position is topped up as shares instead of assetsclaimYield(Payment[] payments)—BOT,nonReentrant,whenNotPaused; onevault.withdrawplus asafeTransferfan-outemergencyWithdraw() returns (uint256 shares)—MANAGER,nonReentrant; transfers all vault shares toprincipalOwnerand zeroesprincipalgetYieldRecipients() view returns (address[]),addYieldRecipient(address),removeYieldRecipient(address)—MANAGERsetPrincipalOwner(address)—DEFAULT_ADMIN_ROLEpause()—PAUSER;unpause()—MANAGEREvents:
PrincipalDeposited,PrincipalWithdrawn,PrincipalIncreased,EmergencyWithdrawn,YieldClaimed,YieldPaid,AddYieldRecipient,RemoveYieldRecipient,SetPrincipalOwner.Errors:
ZeroAddress,ZeroAmount,AlreadySet,NoYieldRecipient,NotYieldRecipient,DuplicateRecipient,RecipientNotFound,ExceedsPrincipal,ExceedsClaimable,ZeroShares,WithdrawShortfall,SharesRemaining.Storage layout
N/A for collision analysis — new contract, first deployment, no prior implementation to be compatible with.
For future upgrades: the contract inherits
UUPSUpgradeable,AccessControlEnumerableUpgradeable,PausableUpgradeableandReentrancyGuardUpgradeablefromopenzeppelin-contracts-upgradeablev5.2.0, all of which use ERC-7201 namespaced storage (e.g.AccessControlEnumerableStorageLocation = 0xc1f6…2000), so no parent occupies a sequential slot and no__gapis required. This contract's own variables occupy slots 0-5 in declaration order:New variables in a future implementation must be appended after
yieldRecipients.Access control
New contract, so every gate is new. Points a reviewer should check deliberately:
_authorizeUpgradeisonlyRole(DEFAULT_ADMIN_ROLE)and empty; the upgrade key is the protocol TimeLock, not the multisig.emergencyWithdrawisonlyRole(MANAGER), notDEFAULT_ADMIN_ROLE. A 24-hour TimeLock-gated exit is not an emergency exit, and the funds areprincipalOwner's own corpus. The destination is pinned toprincipalOwnerand cannot be passed in;setPrincipalOwnerremainsDEFAULT_ADMIN_ROLE-only, so MANAGER cannot redirect the exit. DEFAULT_ADMIN keeps the upgrade key and MANAGER role administration, so it can self-grant if B0c6 is unavailable.emergencyWithdrawis notwhenNotPaused— the incident that pauses this contract is the reason to call it.withdrawPrincipaliswhenNotPaused: a halted contract must not let principal leave piecemeal, so while the pause holds the emergency exit is the only route out, and it cannot be walked down amount by amount. MANAGER holdsunpause, so this is a speed bump for MANAGER rather than a trap.claimYieldcan only pay whitelisted destinations, and the whitelist isMANAGER-only._disableInitializers(), so the implementation itself can never be initialized.Risk assessment
principalOwner; yield can only leave to whitelisted destinations, capped byclaimableYield().principalis an explicit baseline that never floats with NAV, so a NAV drop cannot be harvested as yield.initializesets the baseline at deployment, so the launch share transfer needs nothing else —increasePrincipalis NOT part of the launch, and calling it on top would count the same corpus twice against a baseline that cannot be lowered again. Residual operational risk applies to LATER share top-ups only: there the share transfer andincreasePrincipalmust ride in ONE Safe MultiSend, or the whole transferred position reads as claimable yield in between. Documented in NatSpec onincreasePrincipal.principalOwnerat will but cannot change that destination. Upgrade authority sits with the TimeLock.MoolahVaultandSafeERC20transfers of lisUSD / vault shares. All state-changing entrypoints arenonReentrant.principalis zeroed before the share transfer inemergencyWithdraw, and the post-transfer balance is re-read (SharesRemaining) because SafeERC20 checks the return value, not the effect.Deployment-time front-running is closed structurally, which is the direct answer to the 2026-05-25 lisAster proxy hijack (root cause: a non-atomic deploy script that left a front-runnable window between deployment and initialization / ownership transfer):
initializecalldata rides in theERC1967Proxyconstructor, so no block exists in which the proxy is deployed but uninitialized._implscript asserts this with a localcall.initialize— nograntRole/revokeRolewindow, and the deployer never holds a role.abi.encodeCalltype-checks the initializer arguments at compile time, and the post-broadcastrequireblock asserts the resulting state.Deployment
script/utils/deploy_moolahVaultAccount.s.sol:DeployMoolahVaultAccountdeploys implementation plus initializedERC1967Proxyin one run;script/utils/deploy_moolahVaultAccount_impl.s.sol:DeployMoolahVaultAccountImpldeploys an implementation only, for later TimeLock-driven upgrades (it must never touch the proxy).PRIVATE_KEYvia the existingDeployBase._deployerKey();BSC_RPCfor the fork tests.--broadcast), both scripts SIMULATION COMPLETE:The post-broadcast self-checks run inside the simulation: ERC-1967 implementation slot, all four role holders, member count 1 per role, deployer holds neither DEFAULT_ADMIN nor MANAGER,
vault/principalOwner/principal, and two seeded yield recipients.Not in this PR: the two operational steps that follow deployment — whitelisting the proxy on the vault through the Lending TimeLock (24-hour
schedule→execute), and moving ≈28.27 M shares out of B0c6. The share transfer is the whole of that step:initializehas already setprincipalto 28,300,000, soincreasePrincipalmust NOT be called at launch. An audit is planned before that share transfer.Test plan
forge test --mc MoolahVaultAccountTest→74 passed; 0 failed; 0 skippedforge test --mc MoolahVaultAccountForkTest→8 passed; 0 failed; 0 skipped(BSC fork at block 116,433,631)npm run check→All matched files use Prettier code style!--broadcastand pass their ownrequireself-checksErrorsLib.NotEnoughLiquidityon an over-liquidity withdrawal,NotWhiteListon an un-whitelisted deposit, and that a share transfer into this contract needs no whitelistemergencyWithdrawmoves shares at any liquidity level — a share transfer never walks the withdraw queue, so the exit is not bounded by vault liquidityLocal caveat, unrelated to this PR:
test/utils/PositionMigrator.t.solcannot compile in a git worktree because thelib/lista-dao-contracts.gitsubmodule is not materialized there, so local runs used--skip PositionMigrator.t.sol. CI checks out submodules normally and is unaffected.