Steakhouse Financial: Vault v2 Supervisor

Description

Timelocked actions are keyed by the exact bytes data in executableAt[data]. Since Solidity ignores trailing calldata or dirty bytes for decoding but keeps it in msg.data, the owner can submit a valid action with extra trailing bytes. That action executes normally, but helper functions that reconstruct canonical calldata cannot find and revoke it.

This breaks:

• revokeGuardianRemoval and revokeVaultOwnerChange (they encode clean calldata)
• isGuardianBeingRemoved (it also encodes clean calldata, so it returns false)

Note: the action is still revokable via the generic revoke(bytes data) if the exact submitted data (including the extra bytes) is known.

Proof of Concept

function test_ImpossibleRevokeGuardianRemoval() public {        supervisor.addGuardian(address(vault), GUARDIAN);
        bytes memory data = abi.encodePacked(abi.encodeWithSelector(VaultV2Supervisor.removeGuardian.selector, vault, GUARDIAN), hex'11'); // Appending random data at the end of the calldata        supervisor.submit(data);
        vm.startPrank(GUARDIAN);        vm.expectRevert();        supervisor.revokeGuardianRemoval(vault, GUARDIAN);        vm.stopPrank();
        vm.warp(block.timestamp + TIMELOCK);
        address[] memory guardiansBefore = supervisor.getGuardians(address(vault));
        (bool success, ) = address(supervisor).call(data);        require(success);
        address[] memory guardiansAfter = supervisor.getGuardians(address(vault));
        vm.assertEq(guardiansAfter.length, guardiansBefore.length - 1); // Number of guardians reduced    }

Recommendation

Make sure the data that is being received for submission does not include dirty bytes or trailing calldata.

Cantina Managed: The Steakhouse team mitigated this issue by validating, at submission time, that the calldata has the expected length and contains no dirty bytes, using the _decodeCanonicalTwoAddressCalldata function.

Description

A malicious owner can submit many timelocked actions, forcing guardians to revoke them individually. Since revocation is per exact calldata entry, guardians pay O(n) gas to clean up n scheduled actions, matching the owner’s griefing budget and potentially making defense impractical during an incident.

Recommendation

Add an O(1) circuit breaker, for example a pause mechanism or global “revoke all pending actions” switch (optionally per vault), so guardians can halt execution without needing to revoke every scheduled item individually.

Cantina Managed: Verified the fixes. There is now a single canonical calldata that corresponds to a timelocked action, so guardians only need to revoke one calldata per action if required.

Description

An allowlisted vault owner (or the supervisor owner) can call addGuardian across a large number of vault addresses, growing _vaults and _guardians. This can make getVaults, getOwnedVaults, getNonOwnedVaults, and per-vault guardian enumeration increasingly expensive and potentially unusable for onchain callers.

Recommendation

Consider whitelisting vaults instead of vault owners so that number of vaults can't suddenly grow.

Steakhouse: This behavior is intentional, and we accept the associated trade-offs.

Cantina Managed: Acknowledged.

Trust Assumptions

This system relies on several strong trust assumptions around privileged roles and vault configuration.

Guardian Role

The guardian set is fully trusted. If an incorrect or malicious address is added as a guardian, the system can be irreversibly compromised. Guardians are expected to act honestly and defensively. In particular, while the owner may become malicious, guardians are assumed to monitor and intervene where possible to prevent harmful actions.

Owner and Sentinel Control

For vaults owned by the VaultV2Supervisor, guardians are always able to configure the supervisor as the vault’s sentinel and retain access to revoke functionality. This ensures an emergency control path remains available.

For vaults not owned by the VaultV2Supervisor, this assumption no longer holds. The vault owner can remove the supervisor from the sentinels list, thereby blocking its ability to act as a sentinel or use revoke-related functionality. In such configurations, safety depends entirely on the vault owner’s behavior.

Curator Role and Supervisor Control

The curator role on vaults is fully controlled by the supervisor. While curators are not expected to be able to directly steal funds without going through timelocked mechanisms, they can influence vault parameters in ways that materially affect usability.

Specifically, a curator can modify absolute caps and relative caps in a way that renders the vault effectively unusable for users. This is considered within their authority and is therefore a trust assumption rather than a bug.

Additionally, when the contract only has sentinel access, it cannot call decreaseRelativeCap, decreaseAbsoluteCap, or deallocate. This limits the blast radius of sentinel-only permissions.

Underlying Vault Configuration

All underlying vaults are assumed to be correctly configured according to the intended Morpho Vault V2 design and to have been properly audited. Misconfiguration or unsafe customizations at the underlying vault layer may invalidate the security assumptions of this system.

Scope

Spearbit engaged with Steakhouse to review the vault-v2-supervisor repository at commit 34b2ec2:

In-scope files:

.├── src│   └── VaultV2Supervisor.t.sol└── test    └── VaultV2Supervisor.t.sol

Code Overview

VaultV2Supervisor is a governance wrapper intended to own one or more Morpho Vault V2 instances and provide a supervisory control plane with a global timelock and per‑vault guardian registry. It implements two‑step supervisor ownership transfer, a guardian allowlist for vault owners, timelocked scheduling and execution for specific supervisor actions, and a guardian proxy that can forward vault‑level revoke(bytes) calls.

The supervisor exposes:

• Immediate owner actions to manage vault metadata and roles (e.g., curator, sentinels, name/symbol), set skim recipient on adapters, and manage guardian allowlisting.
• Timelocked actions (setOwner, removeGuardian) that must be submitted, delayed, and then executed, with the ability for guardians or the owner to revoke pending actions.
• Vault tracking helpers (vaults with guardians, owned vs. non‑owned).
• A guardian proxy for Vault V2 timelock revocations, enabled when the supervisor is set as a vault sentinel.

Vault V2 itself distinguishes owner, curator, sentinel, and allocator roles. Owner actions are immediate; curator actions are generally timelocked via the vault’s submit/timelocked mechanism; sentinel and allocator actions are immediate within their respective permission scopes.

Assuming the system is configured safely and all guardians are trusted, the Supervisor owner’s control path across the Supervisor → Vault V2 stack is as follows.

Supervisor layer (direct control)

• Immediate owner actions: setCurator, setName, setSymbol, addSentinel, removeSentinel, setSkimRecipient, setAllowedVaultOwner, addGuardian, and two‑step transferSupervisorOwnership.
• Timelocked and guardian‑revocable actions: setOwner (vault ownership transfer) and removeGuardian, via submit → wait → execute. Guardians or the owner can veto via revoke.

Vault V2 layer (via supervisor as owner/curator)

• Owner actions (immediate at vault): setCurator, setIsSentinel, setName, setSymbol.
• Since curator/sentinel can be updated at supervisor's will, supervisor does not need to wait behind a timelock for below actions (immediate at vault):
  submit (curator), revoke (curator or sentinel), decreaseAbsoluteCap (curator or sentinel), decreaseRelativeCap (curator or sentinel), deallocate (allocator or sentinel but updating sentinel is faster).
• Curator actions (vault timelocked via submit):
  setIsAllocator, setReceiveSharesGate, setSendSharesGate, setReceiveAssetsGate, setSendAssetsGate, setAdapterRegistry, addAdapter, removeAdapter, increaseTimelock, decreaseTimelock, abdicate, setPerformanceFee, setManagementFee, setPerformanceFeeRecipient, setManagementFeeRecipient, increaseAbsoluteCap, increaseRelativeCap, setForceDeallocatePenalty.
• Allocator/sentinel actions are immediate at vault, but for the supervisor to be able to change the allocator, malicious setIsAllocator needs to wait behind a timelock:
  allocate, setLiquidityAdapterAndData and setMaxRate.
  The allocator role itself is granted only via the timelocked curator action setIsAllocator.

Guardian veto path

• Guardians can veto supervisor‑level timelocks (setOwner, removeGuardian) through the supervisor’s revoke.
• Guardians can veto vault‑level timelocks only if the supervisor is a vault sentinel; this is enabled via packing setSupervisorAsSentinel with supervisor.revoke(vault, data) to forward the veto to Vault V2.

In this solo review, Cantina reviewed Steakhouse’s VaultV2Supervisor contract. The team was responsive throughout the process and addressed feedback promptly and thoroughly. Based on the scope of this review and the fixes implemented, the codebase is in good condition and suitable for deployment.

Description

The immutable variable timelock does not follow Solidity’s recommended naming convention for constants and immutables, which should use UPPER_CASE_WITH_UNDERSCORES as specified in the Solidity style guide: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#constants

Recommendation

Rename timelock to TIMELOCK to align with Solidity conventions.

Cantina Managed: Verified the fixes.

Description

The contract repeats the same access check inline instead of using a modifier: _guardians[vault].contains(msg.sender) || msg.sender == owner

This pattern appears in revoke and _revokeKnownVault. The latter is called by revokeGuardianRemoval and revokeVaultOwnerChange, so placing the check on those external entrypoints would improve readability and separation of concerns.

Recommendation

Add an onlyOwnerOrGuardian(address vault) modifier and apply it to relevant functions, especially the external callers of _revokeKnownVault.

Cantina Managed: Verified the fixes.

Description

In submit, decoded variables use abbreviated names: (address v, address newO) = abi.decode(...)

Short names reduce readability and make the intent less clear, especially in security-sensitive logic.

Recommendation

Use descriptive names such as: (address _vault, address _newOwner)

Cantina Managed: Verified the fixes.

Description

_selectorUsesVault includes removeSentinel, but removeSentinel is not timelocked and does not go through the submit flow. This makes the selector list misleading.

Recommendation

Remove removeSentinel from _selectorUsesVault (and any related selector lists used for timelock context).

Cantina Managed: Verified the fixes.

Description

The constructor check require(timelock_ > 0, InvalidTimelock()); only ensures the value is nonzero but does not enforce that it is practically safe. Extremely small values could undermine the purpose of a timelock.

Recommendation

Introduce a minimum bound constant, e.g. MINIMUM_TIMELOCK, and validate against it to ensure the configured delay is meaningful.

Steakhouse: We don't consider this an issue, as we will only deploy one supervisor per chain, and the value is immutable.

Cantina Managed: Acknowledged. Provided the deployment is properly verified, no further changes are required.

Description

revoke(bytes calldata data) duplicates the same core logic implemented in _revokeKnownVault (permission check, timelock existence check, clearing scheduledNewOwner when relevant, clearing executableAt, emitting event). This is unnecessary duplication and slightly increases maintenance risk.

Recommendation

Refactor revoke(bytes) to reuse _revokeKnownVault (or a shared internal helper) so the revoke logic lives in one place.

Cantina Managed: Verified the fixes.

Description

The contract does not follow Solidity’s recommended function ordering, which can make navigation and review harder. The suggested order is documented here: https://docs.soliditylang.org/en/latest/style-guide.html#order-of-functions

Recommendation

Reorder functions to follow Solidity’s standard structure to improve readability and consistency.

Cantina Managed: Verified the fixes.

Description

Once transferSupervisorOwnership(newOwner) sets pendingSupervisorOwner, there is no direct way for the current owner to cancel it. To “cancel,” the owner must overwrite it by proposing another address, which is clunky and can be error-prone.

Recommendation

Add an explicit cancel function (or allow transferSupervisorOwnership(address(0))) to clear pendingSupervisorOwner.

Cantina Managed: Verified the fixes. It is now possible to call transferSupervisorOwnership with address(0) as the calldata to cancel an ongoing transfer process.