fix(utils): MoolahVaultAccount audit follow-up

[qingyang-lista]

Summary

Follow-up to moolah#229 from its audit. Four changes, no new contract and no new external function: withdrawPrincipal now respects the pause, MANAGER becomes the role admin of PAUSER, increasePrincipal's NatSpec is corrected on what the launch sequence actually is, and the implementation deploy script's initializer-lock self-check is made non-vacuous.

Targets feat/moolah-vault-account rather than master so moolah#229 keeps its own review history.

Change type

• New contract
• Upgrade (existing proxy)
• Bug fix
• Gas optimization
• Configuration change (role-admin topology)
• Migration / deploy script
• Test

Contracts changed

Contract	File	Type
MoolahVaultAccount	src/utils/MoolahVaultAccount.sol	modified (pre-deployment; nothing is live yet)
DeployMoolahVaultAccount	script/utils/deploy_moolahVaultAccount.s.sol	modified
DeployMoolahVaultAccountImpl	script/utils/deploy_moolahVaultAccount_impl.s.sol	modified
MoolahVaultAccountTest	test/utils/MoolahVaultAccount.t.sol	modified (test only)

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 deliberately 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 on the corpus.

This inverts the previous stance, and test_withdrawPrincipal_worksWhenPaused was inverted to test_withdrawPrincipal_revertsWhenPaused, which also asserts the emergency exit still works while paused and that MANAGER can lift the pause afterwards.

2. MANAGER is now the role admin of PAUSER

_setRoleAdmin(PAUSER, MANAGER) joins the existing _setRoleAdmin(BOT, MANAGER).

PAUSER at 0xEEfebb1546d88EA0909435DF6f615084DD3c5Bd8 is a Safe with getThreshold() == 1 over 15 owners (the PR #229 body describes it as an EOA; it is not). Any one of fifteen keys can therefore halt claimYield and depositPrincipal. Revoking such a key must not wait on a 24-hour TimeLock proposal, which is the same argument that already put BOT's administration with MANAGER.

DEFAULT_ADMIN_ROLE (the protocol TimeLock) still administers MANAGER, so nothing is removed from the TimeLock's reach.

The main deploy script's post-deploy self-check now asserts both edges:

require(account.getRoleAdmin(account.BOT()) == account.MANAGER(), "bot role admin");
require(account.getRoleAdmin(account.PAUSER()) == account.MANAGER(), "pauser role admin");

3. increasePrincipal NatSpec corrected — it is not part of the launch

The old NatSpec read as if the launch share transfer had to be paired with increasePrincipal in one MultiSend. It does not, and doing it would be harmful: initialize already sets principal to 28,300,000, so calling increasePrincipal(28_300_000e18) after the share transfer would count the same corpus twice. The baseline cannot be lowered again — there is no setter, withdrawPrincipal would revert because the vault cannot deliver 56.6 M, and only emergencyWithdraw (which empties the position and forces the whole launch to be redone) or an upgrade recovers it. claimableYield() would read 0 for good.

The doc now states that the baseline comes from initialize, that B0c6 only transfers shares at launch, and that there are exactly two sanctioned ways to move the baseline afterwards:

1. depositPrincipal / withdrawPrincipal — lisUSD in or out, the baseline follows the funds in the same call, nothing to pair up;
2. a share transfer plus increasePrincipal, in ONE transaction — this is where the Safe MultiSend requirement applies.

No code change; the on-chain behaviour is unchanged and deliberately unguarded, because raising the baseline can only shrink claimableYield() and a bound on it would break legitimate top-ups after a loss.

4. The implementation deploy script's initializer-lock check was vacuous

deploy_moolahVaultAccount_impl.s.sol asserted the freshly deployed implementation cannot be initialized, by calling initialize with deployer in every address slot. Because _vault is then an EOA, initialize reverts at IMoolahVault(_vault).asset() — so require(!initializable) holds whether or not the constructor burned the initializer. An implementation with no _disableInitializers() would have cleared the check.

It now also asserts the revert reason:

require(ret.length >= 4, "implementation reverted without a reason");
require(bytes4(ret) == Initializable.InvalidInitialization.selector, "initializer was not burned");

InvalidInitialization can only come from the initializer modifier, which runs before any argument is touched.

Verified both directions during the audit: a stub with the same initialize prologue and no initializer lock reverts with empty returndata, so the new assertion rejects it, while the real implementation reverts with InvalidInitialization and passes. test_implementation_revertsWithInvalidInitialization pins the selector in the unit suite.

Interface changes

No signature changes. Behavioural changes:

Function	Before	After
withdrawPrincipal	callable while paused	reverts EnforcedPause while paused
getRoleAdmin(PAUSER)	DEFAULT_ADMIN_ROLE	MANAGER

Storage layout

Unchanged — no state variable added, removed or reordered. forge inspect MoolahVaultAccount storage-layout still reports slots 0-5 as vault, asset, principalOwner, principal, isYieldRecipient, yieldRecipients.

Access control

• withdrawPrincipal gains whenNotPaused; emergencyWithdraw does not.
• MANAGER gains administration of PAUSER. MANAGER already administered BOT and already owned the recipient whitelist, and MANAGER is principalOwner, so this does not put any new fund path within its reach — it only lets it rotate a pause key at multisig speed.
• DEFAULT_ADMIN_ROLE still holds the upgrade key, setPrincipalOwner, and administration of MANAGER.

Risk assessment

Area	Risk	Note
Storage collision	🟢 None	No storage change; nothing deployed yet either
Fund safety	🟢 None	No new fund path. withdrawPrincipal is strictly more restricted than before; the emergency exit is untouched
Access control	🟡 Low	PAUSER administration moves from the TimeLock to MANAGER (B0c6, 3-of-6). Deliberate: it buys pause-key rotation speed and mirrors BOT
External call safety	🟢 None	No external call added

Test plan

• forge test --mc MoolahVaultAccountTest → 74 passed; 0 failed; 0 skipped (72 before; test_initialize_setsPauserRoleAdminToManager and test_implementation_revertsWithInvalidInitialization added, test_withdrawPrincipal_worksWhenPaused inverted in place)
• forge 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!
• script/utils/deploy_moolahVaultAccount.s.sol simulates on BSC mainnet without --broadcast, SIMULATION COMPLETE, gas 4,000,742 — the two new role-admin assertions pass
• script/utils/deploy_moolahVaultAccount_impl.s.sol simulates on BSC mainnet without --broadcast, SIMULATION COMPLETE, gas 2,961,101 — the new selector assertion passes

Local caveat carried over from moolah#229: test/utils/PositionMigrator.t.sol cannot compile in a git worktree because the lib/lista-dao-contracts.git submodule is not materialized there, so local runs used --skip PositionMigrator.t.sol. CI checks out submodules normally and is unaffected.

Audit findings not acted on

• withdrawPrincipal has no delivery check (claimYield re-reads the asset balance, emergencyWithdraw re-reads the share balance, withdrawPrincipal does neither). Closed as won't-fix: MoolahVault is a first-party contract and its logic is trusted, so the scenario requires an adversarial upgrade of the vault by the same protocol TimeLock that already holds this contract's upgrade key.
• The main deploy script's require self-checks run after vm.stopBroadcast(), so with --broadcast a wrong constant is already on-chain when a check trips. Accepted: the value is in the dry run, which is always performed first.

🤖 Generated with Claude Code

[hashdit-bot]

Pull Request Review

This Solidity/UUPS smart-contract PR adds pause enforcement to withdrawPrincipal, changes PAUSER role administration from the TimeLock-backed default admin to MANAGER, and corrects increasePrincipal documentation. It also strengthens deployment checks for role topology and disabled implementation initializers, with corresponding tests.

Sensitive Content

No sensitive content detected.

Security Issues

🟡 [MEDIUM] PAUSER role administration is relaxed from DEFAULT_ADMIN_ROLE to MANAGER

File: src/utils/MoolahVaultAccount.sol
Adding _setRoleAdmin(PAUSER, MANAGER) changes the existing inherited grantRole and revokeRole authorization for PAUSER from DEFAULT_ADMIN_ROLE to the less privileged MANAGER role. A compromised MANAGER can now grant arbitrary accounts the ability to pause the contract or revoke all legitimate pausers. The PR describes this as intentional for faster key rotation, but it is still an access-control relaxation that should be explicitly confirmed.
Recommendation: Confirm that delegating PAUSER administration to every MANAGER holder is intended and that MANAGER has security controls appropriate for this authority. If rapid rotation is needed without granting full PAUSER administration to MANAGER, consider a dedicated pause-administrator role or a narrowly scoped emergency revocation mechanism.

---

Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.