OpenTrade: LytPool

Description

setIssuerAdmin validates ISSUER_ADMIN_ROLE against the service configuration but persists the chosen address locally in the controller. Since the check only runs at write time, the two can drift apart: once an address is granted the role and set here, later revoking it in the configuration has no effect on the controller. There is also no path to clear _issuerAdmin from the controller, so once set it cannot return to the fallback unless a new valid holder is assigned.

Recommendation

Re-validate the role at the point of use (or on read) rather than only at write time, and add a way to clear _issuerAdmin back to the issuer fallback.

OpenTrade: addressed in the issuerAdmin function check https://github.com/opentrade-io/LytPool/commit/26e6ed9353c0d2f2c94b23c3350d35180fa8be77

Cantina Managed: Verified fixes, issuerAdmin() now re-validates the role on every read and clearIssuerAdmin() restores the issuer fallback.

Description

Every test exercises the issuerAdmin == issuer fallback; setIssuerAdmin is never called, so a diverged issuer admin — the core behavior this change introduces, is never covered. The suite would pass unchanged if deposits were still gated by onlyIssuer.

Recommendation

Add coverage for issuerAdmin != issuer:

• after setIssuerAdmin(newAdmin), the new admin can deposit and the previous issuer is rejected with NotIssuerAdmin
• setIssuerAdmin reverts for a non–protocol-admin caller, while paused, and for a target that doesn't hold ISSUER_ADMIN_ROLE
• an operator without PAUSER_ROLE cannot call setPaused

OpenTrade: Addressed in commit https://github.com/opentrade-io/LytPool/commit/26e6ed9353c0d2f2c94b23c3350d35180fa8be77

Cantina Managed: Verified fixes, the new tests cover the diverged issuer admin, all setIssuerAdmin revert paths, and the operator-without-PAUSER_ROLE case.

Scope

Commit 199ec6b on branch feat/pauser-issuer-admin-role.

The assumption of this review is that the already existing code is working correctly and mechanisms such as upgrade are running with no issues. The only reviewed changes are the ones that are introduced in the commit 199ec6b.

Files changed under contracts/:

contracts├── LYTPool.sol├── LYTPoolServiceConfiguration.sol├── controllers│   ├── LYTPoolController.sol│   └── interfaces│       └── ILYTPoolController.sol└── interfaces    └── ILYTPoolServiceConfiguration.sol

Code overview

Commit 199ec6b adds a pauser role and a per-pool issuer admin, then re-gates who can call which functionality.

• LYTPoolServiceConfiguration: new PAUSER_ROLE and ISSUER_ADMIN_ROLE constants, an onlyPauser modifier, and an isIssuerAdmin() view. setPaused moves from onlyOperator to onlyPauser.
• LYTPoolController: new per-pool _issuerAdmin with an issuerAdmin() getter that falls back to the issuer when it's unset. Adds setIssuerAdmin() (protocol admin only, target must hold ISSUER_ADMIN_ROLE), an onlyIssuerAdmin modifier, and _isIssuerAdmin(). onlyIssuerOrAutomation now checks the issuer admin instead of the issuer. setFeeCollectorAddress, setTreasuryWalletAddr and addAdjustmentAmount move from onlyIssuer to onlyIssuerAdmin; setIndicativeAndCollateralRates moves from onlyIssuer to onlyIssuerOrAutomation.
• LYTPool: new onlyIssuerAdmin modifier that reads controller.issuerAdmin(), put on changeRedemptionDestination, depositOffChain and depositFromTransfer (were onlyIssuer).
• Interfaces: issuerAdmin() added to ILYTPoolController, isIssuerAdmin() to ILYTPoolServiceConfiguration.

In the above changes, the team has informed Cantina that the IssuerAdmin role is most likely (but not necessarily) is going to be a timelock.

Description

setPaused gates both pausing and unpausing behind a single PAUSER_ROLE. Now that this is no longer tied to operators, the two actions don't warrant the same trust: pausing is an emergency action that benefits from broad, fast access, while unpausing is deliberate and should be more tightly held.

Recommendation

Consider separating into two different roles. One role as broader role to pause, and a narrower one to unpause.

OpenTrade: This was brought up to the team and requires a larger discussion. For now, we will probably defer this suggestion to a later date.

Cantina Managed: Acknowledged, deferred by the team; setPaused remains gated by the single PAUSER_ROLE.

Below are smaller issues that are batched up together as a list of small nit issues throughout the codebase:

• PAUSER_ROLE is declared apart from the other role constants: LYTPoolServiceConfiguration.sol#L41. Move it up beside the others for consistency.
• No isPauser view: LYTPoolServiceConfiguration.sol#L143. Every other role exposes an is* getter; add isPauser(address).
• _issuerAdmin is a misleading name: LYTPoolController.sol#L67. With an existing issuer role, this reads as that role's admin. Rename it to something clearer.
• onlyIssuerOrAutomation no longer matches its check: LYTPoolController.sol#L104. It now checks the issuer admin, so rename it to onlyIssuerAdminOrAutomation.
• onlyIssuer is unused: LYTPoolController.sol#L131. After the re-gating, onlyIssuer is no longer applied to any functions in the controller, consider removing it.

OpenTrade: addressed in commit https://github.com/opentrade-io/LytPool/commit/26e6ed9353c0d2f2c94b23c3350d35180fa8be77

Cantina Managed: Verified fixes, all five nits are addressed.