Migration

The administrative setters for the Merkle root and migration cap are gated by onlyEnabled. If the migrator is disabled for incident response, governance cannot update these values without first re enabling the policy. That creates a window where the old configuration is live or forces operators to re enable before fixing configuration.

function setMerkleRoot(bytes32 merkleRoot_) external onlyEnabled
onlyAdminOrLegacyMigrationAdmin {
_currentMerkleNonce++;
merkleRoot = merkleRoot_;
emit MerkleRootUpdated(merkleRoot_, msg.sender);
}
function setMigrationCap(uint256 cap_) external onlyEnabled onlyAdminRole {
_setMigrationCap(cap_);
}

The impact is reduced operational flexibility during a pause scenario.

Example: a bad merkle root is published that excludes a large set of eligible holders. The team disables the migrator to stop further migrations and wants to update the merkle root (or reduce the cap). Because both setters are gated by onlyEnabled, they must re-enable the migrator to apply the fix, re-opening migrations under the broken configuration.

Allow these admin setters to run while disabled, or provide separate admin only functions that can update configuration without re enabling the migrator. Keep migrate itself gated by onlyEnabled.

Olympus Team: The issue was resolved in PR#205.

Proof of concept: PoC

MigrationProposalHelper always migrates the full OHMv1ToMigrate amount via the legacy TokenMigrator. If this configured amount exceeds the migrator’s actual capacity (bounded by its gOHM balance and oldSupply headroom), the legacy migrator reverts and the helper activation fails.

Even if the amount was calibrated at planning time, a gOHM balance decrease before activation (e.g., legacy users migrating) creates the same failure mode. This can cause the governance proposal to fail if OHMv1ToMigrate is mis‑calibrated or capacity drifts.

Example flow

1. Ops compute capacity off‑chain and set OHMv1ToMigrate via the setup batch.
2. Ops mint tempOHM based on that stored amount.
3. A normal user migrates through the legacy TokenMigrator, reducing its gOHM balance.
4. Proposal executes activate(), which calls the legacy migrator for the full (now‑stale)

OHMv1ToMigrate and reverts due to insufficient capacity.

Compute and cap OHMv1ToMigrate to the migrator’s capacity before execution (e.g., min(gOHM balance → OHMv1, oldSupply - currentSupply) with a small buffer), and include this check in the runbook or setup scripts.

Olympus Team: Partially Resolved.

MigrationProposal._validate requires the time lock to hold zero OHMv2, OHMv1, gOHM, and tempOHM balances after execution:

require(IERC20(OHMv2).balanceOf(timelock) == 0, "There should be no OHMv2 left in the Timelock");
require(IERC20(GOHM).balanceOf(timelock) == 0, "There should be no gOHM left in the Timelock");

The _validate function runs on-chain as part of proposal execution (not just simulation). An attacker can grief the migration proposal by:

1. Observing the migration proposal in the governance queue
2. Donating 1 wei of OHMv2 (or gOHM) to the timelock address
3. When the proposal executes, _validate() reverts because balanceOf(timelock) != 0
4. The entire governance proposal fails

Impact: DoS of migration governance proposal, requiring resubmission and new governance vote. The validation is overly strict—it checks tokens that are not part of the migration flow (OHMv2/gOHM can exist in the time lock for unrelated reasons). Only tempOHM is migration-specific and should be zero after activation.

1. Remove OHMv2 and gOHM balance checks from _validate() (they are not migration-specific)
2. Or use delta-based assertions: record pre-execution balances and assert no net increase
3. Or add a sweep mechanism to handle unexpected donations before validation

Olympus Team: The issue was resolved in PR#203.

The helper transfers the entire tempOHM balance from the timelock, deposits only the computed amount into the legacy treasury, and then burns any remaining tempOHM. If the timelock holds extra tempOHM for any reason, that surplus is destroyed during activation.

ERC20(TEMPOHM).safeTransferFrom(owner, address(this), ERC20(TEMPOHM).balanceOf(owner));
uint256 tempOHMToDeposit = getTempOHMToDeposit();
ERC20(TEMPOHM).safeApprove(TREASURY, tempOHMToDeposit);
ohmV1Minted = IOlympusTreasury(TREASURY).deposit(tempOHMToDeposit, TEMPOHM, 0);

uint256 excessTempOHM = ERC20(TEMPOHM).balanceOf(address(this));
if (excessTempOHM > 0) {
ERC20Burnable(TEMPOHM).burn(excessTempOHM);
}

The impact is potential destruction of tempOHM balances beyond the intended migration amount.

Transfer only the required tempOHM amount, or explicitly document that any extra tempOHM in timelock will be burned and ensure operational processes keep the balance tight.

Olympus Team: The issue was resolved in PR#202.

The helper always calls addCategory("migration") before burning. The Burner policy reverts if the category is already approved.

If the category was added earlier by any operational test or previous attempt, the helper activation will revert and the governance execution will fail even though all other steps are correct.

Burner(BURNER).addCategory(MIGRATION_CATEGORY);
if (categoryApproved[category_]) revert Burner_CategoryApproved();

Example: the DAO runs a dry-run proposal that adds the "migration" category in Burner. Later, when the real proposal executes on mainnet (or when the helper is re-deployed for a second attempt after a failed run), addCategory("migration") reverts because the category already exists and the activation transaction fails even though everything else is correct.

Make the category addition idempotent. Check whether the category is already approved before adding it, or use a unique category name for this specific migration to avoid collisions.

Olympus Team: The issue was resolved in PR#204.

The Tenderly simulation path in OlyBatch hardcodes the from address to daoMS, even when the batch is intended to be executed by the policy or emergency safe. In OlyBatch, the real sender is safe, which is set by the modifier used (isDaoBatch, isPolicyBatch, isEmergencyBatch).

By forcing daoMS into the Tenderly payload, the simulation can succeed under DAO permissions even though the actual sender would fail on-chain.

This is an operational/simulation mismatch. It can mislead reviewers into thinking a batch is safe to execute when the real sender lacks the required roles or balances.

Example: a batch guarded by isPolicyBatch is supposed to be executed by the policy multisig.

Tenderly simulation runs with daoMS as the sender, passes due to DAO permissions, and the team believes the batch is safe. When executed from the policy multisig, the transaction can revert due to missing roles, causing an avoidable deployment or governance delay.

Note: the newer BatchScriptV2 uses _owner as the sender in its Tenderly payload, which matches the real sender. This issue is specific to OlyBatch and was updated during this PR review (commit e255afe).

Use the actual safe address (the batch sender) for the Tenderly payload from field instead of always daoMS.

Olympus Team: The issue was resolved in PR#208.

The migration helper is a permanent contract with no method to recover tokens sent to it accidentally. Any ERC20 transferred to the helper outside of the activation flow will remain stuck forever.

Add an onlyOwner sweep function for arbitrary tokens, or explicitly document that the helper must never receive tokens outside the activation sequence.

Olympus Team: The issue was resolved in PR#209.

The migration cap logic is implemented by synchronizing MINTR mintApproval to a target value. Because MINTR approvals are consumed when minting, the value represents remaining allowance rather than a lifetime total.

The function name and comments can be interpreted as setting an absolute cap, which can lead to operational mistakes if governance resets the approval without accounting for already minted amounts.

The impact is governance confusion and potential misconfiguration of the migration allowance.

Example: 600 OHM v2 has already been minted and the remaining approval is 400. Governance calls setMigrationCap(1000) expecting the total cap to be 1000 overall.

The code sets the remaining approval to 1000, allowing another 1000 to be minted, for a total of 1600. The system behaved correctly, but the operator expectation was wrong because the cap is “remaining allowance,” not “lifetime total.”

Clarify semantics in naming and documentation, or track a separate total cap in V1Migrator and enforce minted total against it. If using remaining approvals, rename to something like setRemainingMintApproval and update NatSpec accordingly.

Olympus Team: The issue was resolved in PR#210.

Migration converts OHM v1 to OHM v2 by calling gOHM.balanceTo and then gOHM.balanceFrom. Both conversions round down. If a user migrates in multiple small transactions, dust can be lost on each conversion rather than once on the total amount.

uint256 gohmAmount = _GOHM.balanceTo(amount_);
uint256 ohmV2Amount = _GOHM.balanceFrom(gohmAmount);
if (ohmV2Amount == 0) revert ZeroAmount();

function balanceFrom(uint256 _amount) public view override returns (uint256) {
return _amount.mul(index()).div(10**decimals());
}
function balanceTo(uint256 _amount) public view override returns (uint256) {
return _amount.mul(10**decimals()).div(index());
}

The impact is minor value loss for users who migrate in many small calls.

Add a previewMigrate view helper and document that users should migrate in a single transaction when possible. Consider adding a minimum amount threshold or UI guidance to reduce dust loss.

Olympus Team: The issue was resolved in PR#207.

The incurDebt function increases reserveDebt and totalDebt by amount_ before transferring tokens to the debtor. If the system supports fee-on-transfer or deflationary tokens, the debtor receives less than amount_, but debt is recorded for the full amount.

This overcharges the debtor and causes getReserveBalance (balance + totalDebt) to overstate total reserves by the amount lost to fees.

Either reject fee-on-transfer/deflationary tokens for debt operations, or measure the actual amount sent to the debtor (e.g., recipient balance delta) and record debt accordingly. Alternatively, perform a pre/post balance check on the treasury and adjust debt to the net amount that leaves the treasury.

Olympus Team: Acknowledged.

setBridgeStatus(false) only affects sendOhm (outbound burns). Inbound minting via lzReceive/retryMessage continues even when the bridge is “deactivated,” because neither path checks bridgeActive.

This makes the kill‑switch asymmetric: operators may believe a shutdown halts minting, but messages from trusted remotes will still mint OHM on the destination chain.

If shutdown should stop all bridge activity, add bridgeActive checks to lzReceive and retryMessage (or explicitly document that deactivation is outbound‑only and use trusted‑remote removal to halt inbound).

Olympus Team: Acknowledged.

retryMessage only verifies the stored payload hash and then calls _receiveMessage directly. It does not re‑check whether the trusted‑remote entry is still configured. As a result, a previously failed payload can be replayed to mint OHM even after operators clear trusted remotes.

Re‑validate trusted remote/endpoint constraints (or explicitly document that retries bypass these controls). Consider invalidating stored payloads when removing trusted remotes.

Olympus Team: Acknowledged.

setMerkleRoot increments the migration nonce and replaces the root, without validating that the new root differs from the current one. Existing proofs for the previous root become invalid, and per‑user migrated amounts reset for the new nonce.

A mid‑campaign root update can block users until new proofs are generated and distributed. If the same root is accidentally set again, the nonce still increments and migrated tracking resets, allowing a full re‑migration under the same tree.

Add a guard to reject setMerkleRoot when merkleRoot_ == merkleRoot (or otherwise require an explicit “reset” flag), and document that root updates are campaign resets. If keeping the current behavior, update the runbook to warn that same‑root updates re‑open migrations.

Olympus Team: The issue was resolved in PR#206.

The governance proposal approves a fixed tempOHM allowance based on the timelock balance at proposal build time, but the helper later transfers the timelock's full balance at execution time. If the timelock balance changes between those two moments, the transferFrom can exceed the approved amount and revert, causing the entire proposal execution to fail.

Operationally, the migration plan expects the timelock to hold only the intended amount of tempOHM and to have that balance remain unchanged until execution. That expectation is not enforced in code. tempOHM is a standard ERC20 and is transferable, so any holder can donate dust to the timelock and increase the balance.

The same effect occurs if the tempOHM owner mints additional tokens to the timelock after the proposal is queued. In either case the helper tries to pull the new, higher balance even though the allowance was set to the old balance, and the proposal reverts.

This does not imply a loss of protocol funds and may never happen if the team strictly ensures that the timelock is the only tempOHM holder and no additional minting occurs. The point is that this safety relies on an off-chain assumption rather than an on-chain invariant and the failure mode is a governance execution revert that requires a re-submission.

Make the transfer deterministic relative to the approved amount. The simplest approach is to approve type(uint256).max and revoke to zero after execution. Alternatively, transfer only the expected amount (for example, getTempOHMToDeposit or min(balance, allowance)) and handle any excess in a separate, explicit burn step. This removes the dependency on timelock balance stability and makes execution robust even if the balance drifts.

Olympus Team: The issue was resolved in PR#214.

The newly added rescue() function has the onlyEnabled modifier, meaning tokens cannot be rescued when the contract is disabled/paused. This is counterintuitive for a rescue function — emergencies requiring pause are exactly when stuck tokens might need recovery.

Scenario:

1. Emergency discovered → admin calls disable() to pause migrations
2. Tokens need to be rescued (e.g., accidentally sent or from exploit recovery)
3. rescue() reverts due to onlyEnabled
4. Admin must re-enable (potentially exposing users to the emergency) just to rescue tokens

Contrast: setMerkleRoot() and setRemainingMintApproval() correctly removed onlyEnabled to allow admin updates while paused.

Remove onlyEnabled from rescue(). The access control (onlyAdminOrLegacyMigrationAdmin) is sufficient protection. This aligns with the pattern used for setMerkleRoot() and setRemainingMintApproval() which were updated to allow calls while disabled.

Olympus Team: The issue was resolved in PR#213.