Add Admin Role to Vault by shahthepro · Pull Request #2973 · OriginProtocol/origin-dollar

shahthepro · 2026-08-13T12:54:15Z

Code Changes

Adds a new adminAddr slot for the OToken Vault
Adds deployment files for OUSD, OETH and OETHb Vaults
Set the admin address to 5/8
Changes the roles on pauseCapital/unpauseCapital and pauseRebase/unpauseRebase methods (2/8 can only call pause. 5/8 can unpause)
Adds a new PauseSafeModule that sits on top of 2/8 and can be used by HyperNative to call pause immediately on our contracts

Code Change Checklist

To be completed before internal review begins:

The contract code is complete
Executable deployment file
Fork tests that test after the deployment file runs
Unit tests *if needed
The owner has done a full checklist review of the code + tests

Internal review:

Two approvals by internal reviewers

Deploy checklist

Two reviewers complete the following checklist:

- [ ] All deployed contracts are listed in the deploy PR's description
- [ ] Deployed contract's verified code (and all dependencies) match the code in master
- [ ] Contract constructors have correct arguments
- [ ] The transactions that interacted with the newly deployed contract match the deploy script.
- [ ] Governance proposal matches the deploy script
- [ ] Smoke tests pass after fork test execution of the governance proposal

sparrowDom

Left comments inline

sparrowDom · 2026-08-13T14:27:44Z

+    ///      Guardian Safe hosting this module) and the native staking strategies.
+    ///      Any target exposing a no-argument `pause()` is reachable through
+    ///      `pause(address)` below.
+    bytes4 internal constant PAUSE_SELECTOR = 0x8456cb59;


👍 confirmed

sparrowDom · 2026-08-13T15:01:07Z

@@ -0,0 +1,69 @@
+const addresses = require("../../utils/addresses");


🔴 we should be using Foundry for deployment example: https://github.com/OriginProtocol/origin-dollar/blob/master/contracts/scripts/deploy/mainnet/003_DeployOUSD.s.sol

Good catch, will change it

sparrowDom · 2026-08-13T15:05:29Z

+    function _execPause(address _target, bytes4 _selector) internal {
+        require(isPausableTarget[_target], "Target not allowed");
+
+        bool success = safeContract.execTransactionFromModule(


I think this doesn't fail if the target has no code (EOA account)

Have added it to _allowTarget method. While it's true that self destruct can cause this to fail, but I feel that's okay since the Safe manually reviews the contract addresses before whitelisting and any revert due to self-destruct wouldn't cause any fund drains either

sparrowDom · 2026-08-13T15:09:47Z

+      "OUSDVault",
+      [addresses.mainnet.USDC],
+      undefined,
+      true


We should cancel this script for favoring Foundry deploy. Still I think upgrade storage slot safety should pass

sparrowDom · 2026-08-13T16:55:45Z

 ('origin-dollar', 'otoken_os_rebase',                         'cd /app && pnpm exec tsx tasks/run.ts otokenOsRebase --network sonic',                 '45 11,23 * * *',        'UTC', false, NULL),
 ('origin-dollar', 'ogn_claimAndForwardRewards',               'cd /app && pnpm exec tsx tasks/run.ts ognClaimAndForwardRewards --network mainnet',    '50 0 * * 2',            'UTC', false, NULL),
 ('origin-dollar', 'otoken_oethb_harvest',                     'cd /app && pnpm exec tsx tasks/run.ts otokenOethbHarvest --network base',              '55 11 * * *',           'UTC', false, NULL),
-('origin-dollar', 'module_rebase_mainnet',                    'cd /app && pnpm exec tsx tasks/run.ts permissionedRebase --network mainnet',           '15 10,22 * * *',        'UTC', false, NULL),


note: this won't delete the rows in Talos. Maybe a checklist item would be nice so that you remember to Archive them (a group in Talos for actions that aren't used anymore)

sparrowDom · 2026-08-13T16:58:05Z

+    /// @dev `bytes4(keccak256("pause()"))`. Not taken from a project interface
+    ///      because it is shared by contracts that have none in common: the ARMs
+    ///      (`AbstractARM.pause()`, guarded by `onlyPauser`, which includes the
+    ///      Guardian Safe hosting this module) and the native staking strategies.


I am not sure it is wired up to pause the native staking strategies.

It isn't. I missed to fix the comment. Will change it

naddison36

My initial thought was why are we adding a Safe module so pause can be called when we can just allow the Vault Operator to pause directly? The reason is we want multiple Operators to pause so we can delegate pausing to third parties like Hypernative.

That then raises the question of why add the Operator to the Vault just to call rebase. We could use a Safe module. This would remove the Operator complexity from the Vault. If we were starting fresh, I'd remove the Operator role from the Vault and use a Safe module instead. But its in now so I'd prefer to just keep it.

Given the admin (5/8) can pause, I'd prefer the Admin multi-sig had the Safe module to pause so then we have a 5/8 approving third party Operators, not just a 2/8.

It is possible to not do any Vault code changes. We can change the Vault's Strategist to the Admin (5/8) and then use a Safe module to allow the Guardian (2/8) and Operator (EOA) to call the existing Strategist and Operator functions.

My current in two minds as to whether we:

Keep this PR but move the Safe Module to the Admin (5/8) to allow Hypernative to pause.
No Vault changes and change the Vault's Strategist to the Admin (5/8) with a larger Safe module

The PR's proposed mapping of functions to roles:

Function(s)	Governor	Strategist / Guardian Safe (2/8)	Admin Safe (5/8)	Vault Operator	Pause-module Operator	Anyone
`pauseCapital()`	Direct	Direct	Direct	—	Via Guardian Safe	—
`pauseRebase()`	Direct	Direct	Direct	—	Via Guardian Safe	—
`unpauseCapital()`	Direct	—	Direct	—	—	—
`unpauseRebase()`	Direct	—	Direct	—	—	—
`rebase()`	Direct	Direct	—	Direct	—	—
`setVaultBuffer()`	Direct	Direct	—	—	—	—
`setDefaultStrategy()`	Direct	Direct	—	—	—	—
`setRebaseRateMax()`	Direct	Direct	—	—	—	—
`setDripDuration()`	Direct	Direct	—	—	—	—
`depositToStrategy()`	Direct	Direct	—	—	—	—
`withdrawFromStrategy()`	Direct	Direct	—	—	—	—
`withdrawAllFromStrategy()`	Direct	Direct	—	—	—	—
`withdrawAllFromStrategies()`	Direct	Direct	—	—	—	—
`setAutoAllocateThreshold()`	Direct	—	—	—	—	—
`setStrategistAddr()`	Direct	—	—	—	—	—
`setOperatorAddr()`	Direct	—	—	—	—	—
`setAdminAddr()`	Direct	—	—	—	—	—
`setWithdrawalClaimDelay()`	Direct	—	—	—	—	—
`approveStrategy()`	Direct	—	—	—	—	—
`removeStrategy()`	Direct	—	—	—	—	—
`addStrategyToMintWhitelist()`	Direct	—	—	—	—	—
`removeStrategyFromMintWhitelist()`	Direct	—	—	—	—	—
`setMaxSupplyDiff()`	Direct	—	—	—	—	—
`setTrusteeAddress()`	Direct	—	—	—	—	—
`setTrusteeFeeBps()`	Direct	—	—	—	—	—
`transferToken()`	Direct	—	—	—	—	—
`allocate()`	Direct	Direct	Direct	Direct	Direct	Direct
`mint()` / `mintFor()`	Direct	Direct	Direct	Direct	Direct	Direct
`redeem()`	Direct	Direct	Direct	Direct	Direct	Direct
`requestWithdrawal()`	Direct	Direct	Direct	Direct	Direct	Direct
`claimWithdrawal()`	Direct	Direct	Direct	Direct	Direct	Direct

clement-ux · 2026-08-18T08:37:47Z

+    modifier onlyGovernorOrAdmin() {
+        require(
+            msg.sender == adminAddr || isGovernor(),
+            "Caller is not the Admin or Governor"
+        );
+        _;
+    }
+
+    /**
+     * @dev Verifies that the caller is the Governor, Strategist or Admin.
+     */
+    modifier onlyGovernorOrStrategistOrAdmin() {
+        require(
+            msg.sender == strategistAddr ||
+                msg.sender == adminAddr ||
+                isGovernor(),
+            "Caller is not the Strategist, Admin or Governor"
+        );
+        _;
+    }


I think this solves the immediate pause/unpause separation, but I’m concerned about encoding every new operational role directly into each application contract.

With the current approach, adding another actor or capability requires a new storage variable, setter, event, and new combinations of modifiers such as onlyGovernorOrAdmin and onlyGovernorOrStrategistOrAdmin. We also duplicate target and selector allowlists in modules such as PauseSafeModule.

This pattern does not scale well as we extend the same access-control model to OUSD, OETH, OETHb, ARMs, strategies, and future contracts. It also makes it increasingly difficult to answer a basic security question: who can call what, across the entire protocol?

Would it make sense to use a shared, target-aware authority similar to Solmate’s RolesAuthority instead?

It models permissions as:

user -> roles (role, target, selector) -> capability

We could configure a PAUSER role for the Guardian/Hypernative path and an UNPAUSER role for the 5/8, with explicit capabilities for each Vault and ARM:

flowchart LR H[Hypernative] --> P[PAUSER role] G[Guardian Safe] --> P A[Admin 5/8] --> P A --> U[UNPAUSER role] P --> AUTH[Global RolesAuthority] U --> AUTH AUTH -->|pauseCapital / pauseRebase| OUSD[OUSD Vault] AUTH -->|pauseCapital / pauseRebase| OETH[OETH Vault] AUTH -->|pauseCapital / pauseRebase| OETHB[OETHb Vault] AUTH -->|pause| ARMS[ARMs] AUTH -->|unpauseCapital / unpauseRebase| OUSD AUTH -->|unpauseCapital / unpauseRebase| OETH AUTH -->|unpauseCapital / unpauseRebase| OETHB

Loading

The main benefit is not only reducing modifiers and storage fields. It would give us one auditable authorization layer per chain — a single source of truth for protocol-wide permissions.

Instead of reviewing access control independently across every Vault, ARM, strategy, and Safe module, we could inspect one authority and answer:

Which addresses have each role?

Which targets can each role access?

Which selectors can each role call?

What changed when a permission was added or revoked?

This would also allow us to avoid contract-specific Safe modules for narrowly scoped operational permissions. Hypernative or another operator could be assigned a role that can call only the approved pause selectors on explicitly approved targets. The authority itself would provide the caller, target, and selector allowlist currently implemented by PauseSafeModule.

In other words, instead of:

Operator -> SafeModule -> Safe -> Target

we could use:

Operator -> Target -> RolesAuthority authorization check

Adding a new target or operator would become an explicit authority configuration change rather than deploying or modifying a Safe module, enabling it on a Safe, and upgrading application-contract storage and modifiers. This would make permissions easier to review, monitor, test, and reason about globally.

A shared authority does create a larger configuration blast radius, so its ownership and permission-management process would need to be designed carefully. However, I think this is preferable to progressively distributing and duplicating authorization logic across the entire protocol.

This may be a larger architectural change than the immediate PR, but I think it is worth evaluating before establishing another contract-specific role pattern that will be expensive to generalize later.

shahthepro added 2 commits August 12, 2026 17:21

Add Admin role to the Vault

f0aabc7

Add Safe Module

b228677

shahthepro requested review from clement-ux, naddison36 and sparrowDom as code owners August 13, 2026 12:54

Add ARM pausing to the module

225934d

sparrowDom reviewed Aug 13, 2026

View reviewed changes

naddison36 reviewed Aug 14, 2026

View reviewed changes

shahthepro added 3 commits August 14, 2026 14:37

Switch to Foundry and address CR comments

7eb146b

Merge branch 'master' into shah/simplified-admin-unpause

c99c68e

Fix CI

a96133f

clement-ux reviewed Aug 18, 2026

View reviewed changes

		@@ -0,0 +1,69 @@
		const addresses = require("../../utils/addresses");

Uh oh!

Conversation

shahthepro commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Changes

Code Change Checklist

Deploy checklist

Uh oh!

sparrowDom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naddison36 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shahthepro commented Aug 13, 2026 •

edited

Loading