Skip to content

Feat: Dromos escrow#140

Open
08xmt wants to merge 2 commits into
devfrom
feat/dromos-escrow
Open

Feat: Dromos escrow#140
08xmt wants to merge 2 commits into
devfrom
feat/dromos-escrow

Conversation

@08xmt

@08xmt 08xmt commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Add a dromos ecrow capable of staking and withdrawing Dromos LP tokens, while letting the user or beneficiaries claim reward tokens.

@08xmt

08xmt commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

No CRITICAL / HIGH / MEDIUM findings. The three escrow-critical invariants all hold:

  • balance() never over-reports (1:1 gauge) → no over-borrow / bad-debt vector.
  • pay() delivers any amount ≤ balance() exactly → no liquidation DoS from this escrow.
  • _claim can never transfer collateral (UnsafeRewardToken + WrongCollateral are jointly sufficient and complete).

NM-001 — onDeposit reverts on a killed/paused gauge → temporary deposit DoS (LOW)

Location: DromosEscrow.sol:118-123, interacting with Market.deposit (Market.sol:286-293).

function onDeposit() external {
    uint256 tokenBalance = token.balanceOf(address(this));
    if (tokenBalance == 0) return;
    gauge.deposit(tokenBalance);   // reverts NotAlive if Aerodrome gov kills the gauge
}

Feynman Q (Cat 5/6): "What happens on the boundary where the external gauge is in a non-normal state?" Aerodrome Gauge._depositFor reverts NotAlive when voter.isAlive(gauge) is false (gauge killed by Aerodrome governance), and reverts ZeroAmount on a 0 deposit. The if (tokenBalance == 0) return guard (L120) correctly neutralizes the ZeroAmount case — good. But a killed gauge makes gauge.deposit revert, which propagates: Market.deposit calls escrow.onDeposit() whenever callOnDepositCallback == true, so the whole deposit transaction reverts.

Impact: Temporary inability to deposit collateral while (a) the gauge is killed AND (b) the market's callOnDepositCallback flag is on.

Why it stays LOW (cross-checked against Aerodrome + the State map):

  • Aerodrome withdraw and getReward have no isAlive gate — so existing positions remain fully withdrawable, liquidations and pay() are unaffected, and accrued emissions remain claimable even after the gauge is killed. The DoS is confined to new staking, not value recovery.
  • Governance can set callOnDepositCallback = false, after which deposits land unstaked in the escrow, are still counted by balance(), and are still deliverable by pay(). No funds are ever lost or frozen.
  • This is inherent to the "callback stakes into an external gauge" design and matches the posture of ConvexEscrowV2.onDeposit → booster.deposit.

Recommendation: Document the dependency in NatSpec, and/or have onDeposit swallow a failed gauge.deposit (try/catch leaving tokens liquid) so deposits never hard-revert on a killed gauge. If you add a try/catch, emit an event on the catch branch so the silent-stake-failure is observable (cf. prior StakeDaoEscrow NM-002).


NM-002 — initialize accepts beneficiary == address(0); emissions become permanently unclaimable (LOW)

Location: DromosEscrow.sol:81-90.

function initialize(IERC20 _token, address _beneficiary) external {
    if (market != address(0)) revert AlreadyInitialized();
    if (address(_token) != address(stakingToken)) revert WrongCollateral();
    market = msg.sender;
    token = _token;
    beneficiary = _beneficiary;          // no zero-check
    _token.forceApprove(address(gauge), type(uint256).max);
}

Feynman Q (Cat 4): "What is implicitly trusted about _beneficiary?" — that it is a real claimant. If _beneficiary == address(0):

  • onlyBeneficiary and onlyBeneficiaryOrAllowlist both compare msg.sender != beneficiary; msg.sender is never address(0), so no one can ever pass claim, claimTo, or setClaimer.
  • Because setClaimer is itself onlyBeneficiary, the allowlist can never be populated to work around it (the beneficiary ↔ allowlist authority coupling collapses).
  • AERO emissions accrue to the escrow but are permanently stranded.

Reachability: Market.deposit(address user, uint amount) is public with an arbitrary user, so Market.deposit(address(0), x)getEscrow(address(0))initialize(collateral, address(0)). This is self-harm (the caller donates collateral to the zero-keyed escrow that nobody can borrow against or claim from) — not a vector against another user, since for a normal deposit the beneficiary is the user argument and is non-zero. Same class as the prior StakeDaoEscrow NM-007.

Recommendation: if (_beneficiary == address(0)) revert InvalidReceiver(); (or a dedicated InvalidBeneficiary) in initialize. Cheap defense-in-depth; makes the misconfiguration fail loudly at the point of mistake.


NM-003 — Gauge is checked for code but not verified as a genuine Aerodrome gauge (INFO)

Location: DromosEscrow.sol:60-74. The constructor rejects an EOA (_gauge.code.length == 0), zero staking/reward tokens, and stakingToken == rewardToken, then caches everything as immutable. It does not (and cannot cheaply) prove the address is a real Aerodrome gauge with 1:1 staking and the expected getReward/withdraw semantics. The whole audit's safety rests on those semantics. This is acceptable trust-on-deployer — the implementation is deployed by the protocol, not users — but is worth stating explicitly in NatSpec, and worth a fork test against the live Base gauge to lock the assumption (the current suite is mock-only).


NM-004 — Reentrancy posture (INFO, no exploit)

onDeposit is intentionally permissionless and unguarded ("so direct collateral transfers can subsequently be staked"). The external-call boundaries:

  • pay()gauge.withdraw(...) then token.safeTransfer(recipient, amount). A reentrant onDeposit() between withdraw and transfer could re-stake the just-withdrawn tokens and make safeTransfer revert — but the Aerodrome gauge's withdraw is nonReentrant with no callback, and the collateral LP is a standard ERC20 with no transfer hook, so there is no reentry trigger. DoS-only even in theory; no value loss; pay is onlyMarket.
  • _claim()gauge.getReward(this) then rewardToken.safeTransfer(receiver, amount). Re-entry by receiver would re-run _claim, but the reward balance is already 0, so it claims nothing and corrupts no state. AERO is not a hook-bearing token.

No exploitable path. Matches the trust posture of ConvexEscrowV2 / StakeDaoEscrow (prior NM-006). Worth a one-line @dev recording the "trusted gauge, no-callback collateral/reward token" assumption.


NM-005 — Stranded Aerodrome Pool trading fees on unstaked LP (INFO)

Aerodrome Pool (LP) tokens accrue trading fees to their holder via the pool's internal claimable0/claimable1 accounting while the LP is held directly (unstaked). The escrow holds unstaked LP transiently (between a direct transfer and onDeposit, or as a residual). The escrow exposes no Pool.claimFees() path, so any such fees remain as pool-internal dust attributable to the escrow and are never realized. This is dust-level, denominated in token0/token1 (WETH/USDC) — neither collateral (the LP) nor reward (AERO) — so it can never affect balance() accounting or the claim path. Fails safe; noted only for completeness.


@08xmt

08xmt commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

NM-001 is not relevant to security since borrowers can still withdraw.
NM-002 is irrelevant since it's impossible for it to happen in current market contract.
Rest are irrelevant INFO issues

@08xmt

08xmt commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

https://v12.sh/runs/4078/public
F-91345: Claimable Escrow Implementation
"DromosEscrow is documented as a proxy implementation, while Market stores that implementation address and deploys per-user clones from it. The implementation contract itself remains externally initializable because initialize only checks that market is still unset and that the supplied token equals the immutable staking token. The first caller to the implementation address can initialize the standalone implementation with themselves as both market and beneficiary. Once initialized this way, pay authorizes that attacker-controlled market to unstake and transfer staking tokens held by the implementation contract. The normal clones are initialized separately in clone storage by Market, so the exploit is limited to assets mistakenly sent to the implementation address rather than to the intended per-user escrows."

The other finding is high but incorrect, and correctly flagged as likely incorrect.

@nourharidy nourharidy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small optimization. Otherwise LGTM

if (address(_token) != address(stakingToken)) revert WrongCollateral();

market = msg.sender;
token = _token;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as stakingToken? If yes then why are we storing it in another storage slot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants