Skip to content

incentive teller with principal checkpointing, IncentivePool, and rewards claiming - #627

Open
bxmmm1 wants to merge 55 commits into
devfrom
feature/incentive-teller-2
Open

incentive teller with principal checkpointing, IncentivePool, and rewards claiming#627
bxmmm1 wants to merge 55 commits into
devfrom
feature/incentive-teller-2

Conversation

@bxmmm1

@bxmmm1 bxmmm1 commented Mar 23, 2026

Copy link
Copy Markdown

Introduces the incentive teller system — a set of teller upgrades that track per-user principal checkpoints on deposit/withdraw/bridge, integrate with a new IncentivePool contract for off-chain
reward distribution, and add withdrawWithRewards to TellerWithYieldStreaming.

Core features

  • Principal checkpointing: Records cumulativeDeposits, cumulativeWithdrawals, and sharePrice at each deposit/withdraw/bridge so the off-chain reward system can attribute principal without
    replaying historical rates
  • IncentivePool: ERC20 reward pool with ECDSA-signed claims, soft-fail processRewards (returns 0 instead of reverting for invalid/expired/rate-limited claims), paginated claim history, and
    configurable complianceWindow semantics
  • TellerWithYieldStreaming.withdrawWithRewards: Atomically withdraw shares and claim rewards from multiple pools in one transaction
  • Explicit to recipient on deposits: DepositParams.to allows directing deposits to an arbitrary recipient instead of always msg.sender
  • Role-based transfer restrictions: transferAllowedRole requires at least one counterparty in a share transfer to hold a specified role

Architecture changes

  • Library extraction: Extract TellerWithMultiAssetSupportLib, LayerZeroTellerLib, CrossChainTellerLib, and PairwiseRateLimiterLib to reduce bytecode size (CCIP teller: 26,093 → 23,945
    bytes, LZ teller: 26,088 → 23,941 bytes — both under EIP-170 limit)
  • LZ chain management consolidation: Replace allowMessagesFromChain/To, stopMessagesFromChain/To, setChainGasLimit with addChain (full overwrite) and stopMessages (selective disable)
  • Delete TellerWithBuffer and PairwiseRateLimiter: Replaced by buffer helper integration in base teller and PairwiseRateLimiterLib

Security fixes

  • Bind compliance signature to msg.sender to prevent front-running replay
  • Enforce bridge destination compliance on depositAndBridge (previously only checked deposit params, not bridge to)
  • disallowBufferHelper now atomically clears active helpers (previously left disallowed helper executing)
  • Explicit NATIVE deposit guard on depositWithPermit
  • tryRecover for soft-fail signature checks in IncentivePool
  • nonReentrant + requiresAuth on withdrawWithRewards
  • Cache getRateSafe() to eliminate redundant external calls in deposit/bridge paths
  • Use _checkpointPrincipalAtRate with explicit rate so refunds cancel at original deposit-time rate

Test coverage

  • ~8,500 lines of new/updated tests across 17 test files
  • BackendRewardSimulation: end-to-end simulation of off-chain reward calculations
  • PrincipalHistoryFuzz: fuzz tests for principal invariants (monotonicity, no phantom principal)
  • RewardRouting / RewardRoutingNegative: reward claim happy path and failure modes
  • CrossChainTellerCompliance: bridge destination compliance enforcement
  • NativeDepositBridgeSafety: native ETH cannot be lost through bridge functions
  • TellerWithYieldStreamingReentrancy: reentrancy and auth gap coverage
  • LayerZeroTellerWithRateLimiting: consolidated LZ chain management + rate limiting

Deleted Contracts

  • TellerWithBuffer — entire contract removed, buffer helper logic absorbed into TellerWithMultiAssetSupport
  • PairwiseRateLimiter — abstract contract removed, logic moved to PairwiseRateLimiterLib (library, no longer overridable)

Changed External Function Signatures (ABI-Breaking)

  • deposit: (ERC20, uint256, uint256, address)(DepositParams, address, ComplianceData) — shares now mint to params.to instead of msg.sender
  • depositWithPermit: flat params → (DepositParams, PermitData, address, ComplianceData)
  • refundDeposit: new uint256 depositSharePrice param inserted — old deposit hashes are incompatible, pending deposits cannot be refunded after upgrade
  • depositAndBridge: flat params → (DepositParams, address, bytes, ERC20, uint256, address, ComplianceData)
  • depositAndBridgeWithPermit: flat struct → nested DepositParams/PermitData/ComplianceData
  • bridge: added ComplianceData param
  • getInstantlyWithdrawableAmount (IBufferLens): TellerWithBufferTellerWithMultiAssetSupport

Removed External Functions

  • setPermissionedTransfers, allowPermissionedOperator, denyPermissionedOperator → replaced by setTransferAllowedRole(uint8)
  • allowMessagesFromChain, allowMessagesToChain, setChainGasLimit → replaced by addChain(...) (full overwrite)
  • stopMessagesFromChain, stopMessagesToChain → replaced by stopMessages(uint32, bool, bool)
  • setOutboundRateLimits, setInboundRateLimits → replaced by setRateLimits(outbound[], inbound[])

New Mandatory Compliance Flow

  • All deposit/bridge paths accept ComplianceData — if complianceSigner != address(0), valid ECDSA signature bound to msg.sender is required
  • Bridge paths additionally enforce compliance on the bridge to address
  • Opt-out: leave complianceSigner as address(0)

New Side Effects on Existing Paths

  • Every deposit/withdrawal/bridge now writes a principal checkpoint to _principalHistory[user]
  • _afterDeposit/_beforeWithdraw now execute manage calls through the vault when a buffer helper is configured

IncentivePool Integration Notes

  • No pool allowlist in the teller — _processRewards calls user-supplied pool addresses, authorization relies on pool-side requiresAuth
  • Soft-fail design: processRewards returns 0 on all failures (bad sig, expired, rate-limited) — never reverts
  • Replayable signatures: no nonce, replay protection via cumulative accounting — signer key rotation must never reuse retired addresses
  • rescueFunds allows owner to drain reward token

bxmmm1 added 30 commits March 11, 2026 19:18
…and add paginated incentive history

Extract shared logic into TellerWithMultiAssetSupportLib, LayerZeroTellerLib,
and PairwiseRateLimiterLib to reduce code duplication across teller variants.

Consolidate LayerZero chain management: replace allowMessagesFromChain,
allowMessagesToChain, stopMessagesFromChain, stopMessagesToChain, and
setChainGasLimit with addChain (full overwrite) and stopMessages (selective
disable).

Add paginated history retrieval to IncentivePool.

Add comprehensive test coverage for LZ teller rate limiting, principal
history tracking, and reward routing (2,600+ lines).
Use getRateSafe() (base-denominated) instead of getRateInQuoteSafe(asset)
so principal is always tracked in base units regardless of deposit/withdraw
asset. Use mulDivDown on deposits and mulDivUp on withdrawals to prevent
phantom principal accumulation from rounding dust.

Add pool allowlist vulnerability test demonstrating teller calls arbitrary
user-supplied pool addresses without validation.
… tracking and fix compliance window semantics

PrincipalCheckpoint now stores cumulativeDeposits and cumulativeWithdrawals (uint104 each)
instead of a single net cumulativePrincipalInBaseAsset (uint208). This enables the off-chain
reward system to correctly attribute principal across transfers without inflating rewards for
share receivers.

Key changes:
- beforeTransfer hook changed from view to state-mutating, pushing timestamp-only
  checkpoints on share transfers so off-chain can reconcile via Transfer events
- complianceDeadline renamed to complianceWindow with relative duration semantics
  (deadline must be <= block.timestamp + window, not an absolute cap)
- depositAndBridge now records principal checkpoints for cross-chain deposits
- Added SafeCast.toUint104 for overflow protection on cumulative values
- Added fuzz tests for principal invariants (monotonicity, no phantom principal)
- Added reentrancy/auth tests for TellerWithYieldStreaming.withdrawWithRewards
… fix withdrawWithRewards ordering

IncentivePool.processRewards now returns 0 instead of reverting for
invalid/expired/rate-limited claims so that withdrawWithRewards never
reverts due to a failed reward. Bridge operations now create withdrawal
checkpoints to prevent phantom principal on the source chain.
…d require initial maxDeadline

Add nonReentrant + requiresAuth to TellerWithYieldStreaming.withdrawWithRewards
to close the reentrancy/auth gap. Require initialMaxDeadline in IncentivePool
constructor so the pool is never deployed with an unset staleness window.
Extract CrossChainTellerLib to reduce CrossChainTellerWithGenericBridge bytecode.
Optimize _checkpointTransfer to overwrite consecutive transfer-only entries
instead of unbounded array growth.
…lCheckpoint to lib

Adds a `to` field to `DepositParams` so deposits can be directed to an
arbitrary recipient instead of always msg.sender. Updates the decoder to
include the `to` address in sanitizer output. Moves `PrincipalCheckpoint`
struct into TellerWithMultiAssetSupportLib for shared access. Updates all
test call sites to pass the new struct shape.
Extends TellerDecoderAndSanitizer with decoders that extract pool
addresses from RewardData arrays for merkle leaf verification. Includes
integration tests through the full ManagerWithMerkleVerification path.
…imports

IncentivePool._checkSignature now uses ECDSA.tryRecover instead of
ECDSA.recover so malformed signatures return false rather than reverting,
preserving the soft-fail contract of processRewards. Also removes unused
SafeTransferLib and ReentrancyGuard imports from TellerWithYieldStreaming.
… tests

Add comprehensive BackendRewardSimulation test simulating off-chain reward
calculations, and expand PrincipalHistoryFuzz with deposit/withdraw/bridge
flow coverage. Remove duplicate _checkpointPrincipal call in bridge deposit.
Blacklisting is handled at a different layer; the in-contract check
was redundant and added unnecessary storage/gas overhead.

Removes setBlacklisted, the blacklisted mapping, and all related tests.
Change withdraw and withdrawWithRewards in TellerWithYieldStreaming
from public to external since they are not called internally.
Record the exchange rate at checkpoint time so the off-chain reward
system can compute share value without replaying historical rates.
…ansfer restrictions

Delete TellerWithMultiAssetSupportLib and move all structs, errors, events, and logic
directly into TellerWithMultiAssetSupport. Add transferAllowedRole to restrict share
transfers so at least one counterparty must hold a specified role. Make beforeTransfer
view by removing checkpoint writes from transfers. Rename cumulativeRewards to
cumulativeOwed in IncentivePool for clarity.
Verify that native ETH cannot be accidentally lost through bridge functions
after removing the revertOnNativeDeposit modifier. Covers NATIVE sentinel
reverts, ERC20 bridge fee paths, excess msg.value rejection, and DualDeposit
guard integrity.
…ccounting

Replace _checkpointPrincipal with _checkpointPrincipalAtRate so callers
specify the rate used for base-value conversion. This lets refundDeposit
record the withdrawal at the original deposit-time rate, ensuring the
refund exactly cancels the deposit's principal impact instead of being
inflated by subsequent rate changes. Also adds a missing bridge-deposit
checkpoint in CrossChainTellerWithGenericBridge and includes
depositSharePrice in the public deposit history hash.
Prevents depositWithPermit from being called with the NATIVE sentinel
address. The previous defense was indirect (accountant revert on missing
rate), which could be bypassed if the accountant ever mapped the sentinel.
…d code

disallowBufferHelper only flipped the allowlist flag without clearing the
helper from currentBufferHelpers, leaving a disallowed helper actively
executing on every deposit/withdrawal. This was a known gap marked as a
TODO in the buffer helper tests (line 673). The fix atomically zeros out
any active deposit/withdraw helper that matches the disallowed address.

Also removes three unused functions from LayerZeroTellerLib
(allowMessagesFromChain, allowMessagesToChain, setChainGasLimit) and
their associated events that were never called from any contract.
Remove duplicate MessageReceived event from CrossChainTellerWithGenericBridge
(only emitted from CrossChainTellerLib). Add natspec clarifying intentional
depositParams.to ignore in _depositAndBridge. Fix typos in TellerWithMultiAssetSupport.
Add getClaimHistoryPaginated test coverage for IncentivePool.
depositAndBridge was only verifying deposit compliance (depositor,
asset, amount) but not the bridge destination address. The standalone
bridge() enforced this via _verifyBridgeCompliance, creating a bypass
path where a compliant depositor could bridge shares to a non-compliant
address.

Add _verifyDepositAndBridgeCompliance to CrossChainTellerWithGenericBridge
that hashes the bridge `to` into the compliance message, so the signer
explicitly approves the full deposit-and-bridge action in one signature.
…aming

- Add missing event emission on setTransferAllowedRole for off-chain monitoring
- Cache accountant.getRateSafe() to avoid redundant external calls in deposit,
  withdraw, and bridge paths
- Pass currentRate as explicit parameter to _checkpointPrincipalAtRate instead
  of fetching it internally
- Fix misleading mapping parameter name: signatureHash -> messageHash
- Replace vm.recordLogs with vm.expectEmit in transfer role tests
Include msg.sender in the compliance hash alongside depositor, so a
signature issued for caller A cannot be replayed by caller B even when
deposit parameters are identical.
Remove dead using-directives and imports across AccountantWithYieldStreaming,
LayerZeroTeller, LayerZeroTellerWithRateLimiting, CrossChainTellerWithGenericBridge,
and ArcticArchitectureLens. Add natspec to the four admin functions in
LayerZeroTellerLib.
Move ECDSA verification, principal checkpointing, and buffer helper
logic into TellerWithMultiAssetSupportLib to reduce bytecode size of
all child contracts. This brings both ChainlinkCCIPTeller and
LayerZeroTeller under the EIP-170 24,576 byte limit.

- CCIP: 26,093 -> 23,945 bytes
- LZ: 26,088 -> 23,941 bytes

Also fix compliance test signing helper to include msg.sender in hash
(missed in 892d36c) and correct version string expectation.
Pass the already-fetched rate through _afterPublicDeposit and _bridge
instead of each function calling accountant.getRateSafe() independently.
Saves 1-2 external calls per deposit/bridge transaction.
The test expected a revert from share locking but never called
setShareLockPeriod, so the default 0 meant shares were never locked.
@bxmmm1 bxmmm1 changed the title Feature/incentive teller 2 incentive teller with principal checkpointing, IncentivePool, and rewards claiming Mar 24, 2026
bxmmm1 added 4 commits March 25, 2026 09:06
…transfer allowlist

Rename the struct field for clarity and extend _enforceTransferAllowlist
to also check the operator address, so approved operators holding the
transfer role can move shares on behalf of users.
…ted history

Add allowedIncentivePools mapping to TellerWithMultiAssetSupport so
_processRewards rejects non-allowlisted pool addresses instead of
acting as an unrestricted proxy. Remove the on-chain per-user
totalRewardCap (backend controls caps via signed cumulativeOwed).
Replace getPrincipalHistory with getPrincipalHistoryPaginated and
add a balance check in processRewards for graceful soft-failure.
Introduce TellerHistoryHelper to aggregate principal and claim
histories across teller and multiple pools in a single call.
Use RolesAuthority role (default 224) instead of a fixed signer address.
complianceSignerRole uses type(uint8).max as disabled sentinel, matching
the transferAllowedRole convention. Enables multiple signers via role
grants and instant revocation without contract state changes.
Move user history view functions into the existing lens contract and
delete the standalone helper, reducing deployment surface.
@bxmmm1
bxmmm1 marked this pull request as ready for review March 27, 2026 08:27
@greptile-apps

greptile-apps Bot commented Mar 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces principal checkpointing, IncentivePool, and rewards claiming on the teller system. It is a well-architected, extensively tested PR (~8,500 lines of tests) with clean library extractions and solid compliance binding. Three P1 issues need resolution before merging: (1) missing principal checkpoint on bridge receive in _completeMessageReceive, (2) depositAndBridge dropped the revertOnNativeDeposit guard with no replacement, and (3) withdrawWithRewards hard-reverts the entire call (including asset withdrawal) when any pool in rewards[] is not allowlisted, inconsistent with IncentivePool's soft-fail design.

Confidence Score: 3/5

Not ready to merge without addressing the missing bridge-receive checkpoint and the withdrawWithRewards authorization gap

Two P1 issues directly undermine core PR invariants: bridge recipients receive no _principalHistory entry on the destination chain (contradicts the stated 'every bridge writes a checkpoint' guarantee), and withdrawWithRewards hard-reverts the asset withdrawal when rewards[] contains an unauthorized pool (inconsistent with IncentivePool's soft-fail design). The missing native-ETH guard on depositAndBridge is a third P1 by defence-in-depth standards. Extensive test coverage (~8,500 lines), compliance binding improvements, nonReentrant+requiresAuth pairing, and bytecode reductions are all solid — addressing the three P1s would bring this to 4/5.

src/base/Roles/CrossChain/CrossChainTellerWithGenericBridge.sol (missing checkpoint on receive, native-ETH guard), src/base/Roles/TellerWithMultiAssetSupport.sol (withdrawWithRewards revert semantics)

Important Files Changed

Filename Overview
src/base/Roles/TellerWithMultiAssetSupport.sol Major refactor adding DepositParams structs, compliance signer roles, principal checkpointing, buffer helpers, incentive pool allowlist, and transferAllowedRole; withdrawWithRewards reverts the full withdrawal if any pool is unauthorized; depositAndBridge missing explicit native-ETH guard
src/base/Roles/CrossChain/CrossChainTellerWithGenericBridge.sol Refactored to use DepositParams/ComplianceData structs, adds compliance verification on bridge paths, but _completeMessageReceive missing principal checkpoint for bridge recipients and depositAndBridge dropped native-ETH guard
src/base/IncentivePool.sol New ERC20 reward pool with ECDSA-signed cumulative claims, soft-fail processRewards, paginated history, and rescueFunds; rewardSigner defaults to address(0) which requires careful init order
src/base/Roles/TellerWithMultiAssetSupportLib.sol New library extracting compliance verification, principal checkpointing (asymmetric rounding), buffer helper management, and reward processing; return values from processRewards are discarded
src/base/Roles/CrossChain/CrossChainTellerLib.sol New library for burnAndEncode and completeMessageReceive; clean extraction but completeMessageReceive cannot call _checkpointPrincipalAtRate from library context
src/base/Roles/CrossChain/Bridges/LayerZero/LayerZeroTellerLib.sol New library consolidating LZ chain management, validation, and fee checking; gas-limit-zero guard preserved from old inline logic
src/base/Roles/CrossChain/PairwiseRateLimiterLib.sol New library converting abstract PairwiseRateLimiter into a deployable library with storage-pointer-based API; decay math logic unchanged
src/base/Roles/TellerWithYieldStreaming.sol Rebased from TellerWithBuffer to TellerWithMultiAssetSupport; adds withdrawWithRewards override that correctly calls updateExchangeRate before processing
src/base/Roles/CrossChain/Bridges/LayerZero/LayerZeroTeller.sol Major simplification via library extraction; addChain/removeChain/stopMessages consolidated, inline logic delegated to LayerZeroTellerLib
src/base/DecodersAndSanitizers/Protocols/TellerDecoderAndSanitizer.sol Updated to decode new DepositParams/ComplianceData/RewardData ABI; old deposit signatures retained for backward compatibility; new withdrawWithRewards and claimRewards decoders added

Sequence Diagram

sequenceDiagram
    participant User
    participant Teller as TellerWithMultiAssetSupport
    participant Lib as TellerWithMultiAssetSupportLib
    participant Vault as BoringVault
    participant Pool as IncentivePool
    participant Accountant

    Note over User,Accountant: deposit() flow
    User->>Teller: deposit(DepositParams, referralAddress, ComplianceData)
    Teller->>Lib: verifyAndMark(usedSigs, authority, role, window, hash, deadline, sig)
    Lib-->>Teller: compliance OK
    Teller->>Vault: enter(from, depositAsset, amount, to, 0)
    Vault-->>Teller: shares minted to params.recipient
    Teller->>Accountant: getRateSafe()
    Teller->>Lib: checkpointPrincipalAtRate(user, shares, isDeposit=true, rate, rate)
    Teller-->>User: shares

    Note over User,Accountant: withdrawWithRewards() flow
    User->>Teller: withdrawWithRewards(asset, shares, minAssets, to, rewards[])
    Teller->>Teller: beforeTransfer(msg.sender, 0x0, msg.sender)
    Teller->>Vault: exit(to, asset, assetsOut, msg.sender, shares)
    Teller->>Lib: checkpointPrincipalAtRate(user, shares, isDeposit=false, rate, rate)
    loop For each RewardData
        Teller->>Lib: processRewards(allowedPools, rewards, user)
        Lib->>Pool: processRewards(user, cumulativeOwed, deadline, sig)
        Pool-->>Lib: amountSent (0 if soft-fail)
        Pool->>User: transfer(amountSent)
    end
    Teller-->>User: assetsOut

    Note over User,Accountant: bridge() source chain
    User->>Teller: bridge(shareAmount, to, wildcard, feeToken, maxFee, compliance)
    Teller->>Teller: _verifyBridgeCompliance(sender, shareAmount, to, deadline, sig)
    Teller->>Lib: checkpointPrincipalAtRate(msg.sender, shares, isDeposit=false, rate, rate)
    Teller->>Vault: exit(0x0, 0x0, 0, msg.sender, shareAmount) [burn]
    Teller->>LZ: _sendMessage(encodedMsg, options, fee)

    Note over Teller,Vault: _completeMessageReceive() destination chain — NO checkpoint written!
    Teller->>Vault: enter(0x0, 0x0, 0, m.to, m.shareAmount) [mint to recipient]
Loading

Comments Outside Diff (3)

  1. src/base/Roles/CrossChain/CrossChainTellerWithGenericBridge.sol, line 227-234 (link)

    P1 Missing principal checkpoint on bridge receive

    CrossChainTellerLib.completeMessageReceive mints shares to m.to on the destination chain but never calls _checkpointPrincipalAtRate for the recipient. The PR's stated invariant is that every deposit/withdrawal/bridge writes a principal checkpoint, but incoming bridge messages leave no trace in _principalHistory on the destination chain.

    This means off-chain reward calculations relying on _principalHistory to attribute principal would not credit bridge recipients for shares they received via inbound messages. The off-chain system would need to fall back to watching MessageReceived events, which defeats the purpose of the unified checkpoint model.

    The library architecture makes this hard to fix directly in completeMessageReceive since it cannot call an internal function on the calling contract. Consider having _completeMessageReceive in the teller override call _checkpointPrincipalAtRate after CrossChainTellerLib.completeMessageReceive returns:

    function _completeMessageReceive(bytes32 messageId, uint256 message) internal {
        MessageLib.Message memory m = message.uint256ToMessage();
        CrossChainTellerLib.completeMessageReceive(vault, messageId, message);
        uint256 rate = accountant.getRateSafe();
        _checkpointPrincipalAtRate(m.to, m.shareAmount, true, rate, rate);
    }
  2. src/base/Roles/CrossChain/CrossChainTellerWithGenericBridge.sol, line 68-72 (link)

    P1 depositAndBridge drops native-ETH guard without replacement

    The revertOnNativeDeposit(address(depositAsset)) modifier that previously blocked depositAndBridge from accepting params.depositAsset == NATIVE has been deleted entirely. depositAndBridge is still payable, so a user who mistakenly passes NATIVE as the deposit asset will hit a low-level revert inside _erc20Deposit with no informative error message. The equivalent check should be re-added inside _depositAndBridge:

    if (address(depositParams.depositAsset) == NATIVE) {
        revert TellerWithMultiAssetSupport__AssetNotSupported();
    }
  3. src/base/Roles/TellerWithMultiAssetSupport.sol, line 747-778 (link)

    P1 withdrawWithRewards reverts the withdrawal on unauthorized pool

    _processRewards (via TellerWithMultiAssetSupportLib.processRewards) reverts with TellerWithMultiAssetSupport__IncentivePoolNotAllowed if any element of the rewards[] array names a pool not in allowedIncentivePools. This revert rolls back the entire transaction including the asset withdrawal — the user receives nothing and their shares are not burned.

    The IncentivePool's own processRewards is explicitly soft-fail (returns 0 on any failure), creating an asymmetry: the pool never blocks a withdrawal, but a single bad pool address in the caller's array does. Consider skipping (rather than reverting on) unrecognised pools in _processRewards, consistent with the soft-fail design.

Reviews (1): Last reviewed commit: "refactor: consolidate TellerHistoryHelpe..." | Re-trigger Greptile

Comment on lines +67 to +72
/**
* @notice Sets the reward signer
* @param newSigner The address of the reward signer
* @dev Callable by OWNER_ROLE.
* When rotating keys, do not reuse a previously active signer address.
* Reusing a retired key would re-enable any unexpired signatures issued under that key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 rewardSigner defaults to address(0) — dangerous partial-init window

The constructor leaves rewardSigner as address(0). If setMaximumRewardAmountPerClaim is called before setRewardSigner, the pool enters a partially-initialised state. While OZ's ECDSA.tryRecover returns RecoverError.InvalidSignature for a zero-address recovery (making an actual exploit practically impossible), requiring rewardSigner in the constructor would tighten the invariant:

constructor(address _owner, ERC20 rewardToken, uint32 initialMaxDeadline, address initialRewardSigner) Auth(...) {
    if (initialRewardSigner == address(0)) revert InvalidSigner();
    rewardSigner = initialRewardSigner;
    ...
}

Comment on lines +164 to +180
ERC20 feeToken,
uint256 maxFee,
address referralAddress,
ComplianceData calldata compliance
) internal returns (uint256 sharesBridged) {
_verifyDepositAndBridgeCompliance(
msg.sender, depositParams.depositAsset, depositParams.depositAmount, to, compliance
);
{
Asset memory asset = _beforeDeposit(depositParams.depositAsset);
sharesBridged = _erc20Deposit(
depositParams.depositAsset,
depositParams.depositAmount,
depositParams.minimumMint,
msg.sender,
msg.sender,
asset

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 depositParams.recipient is silently ignored in _depositAndBridge

The comment on _depositAndBridge notes that depositParams.recipient is intentionally ignored, with shares always minted to msg.sender before being bridged to the separate to argument. Callers who populate params.recipient expecting it to control the destination-chain recipient will be silently wrong. Consider adding an explicit guard or strengthening the natspec to prominently warn that depositParams.recipient has no effect in the bridge path.

Comment on lines 629 to +680
@@ -490,37 +663,39 @@ contract TellerWithMultiAssetSupport is Auth, BeforeTransferHook, ReentrancyGuar
from = msg.sender;
}

shares = _erc20Deposit(depositAsset, depositAmount, minimumMint, from, msg.sender, asset);
_afterPublicDeposit(msg.sender, depositAsset, depositAmount, shares, shareLockPeriod, referralAddress);
_verifyComplianceSignature(to, depositAsset, depositAmount, compliance);
shares = _erc20Deposit(depositAsset, depositAmount, params.minimumMint, from, to, asset);
uint256 rate = accountant.getRateSafe();
_checkpointPrincipalAtRate(to, shares, true, rate, rate);
_afterPublicDeposit(to, depositAsset, depositAmount, shares, shareLockPeriod, referralAddress, rate);
}

/**
* @notice Allows users to deposit into BoringVault using permit.
* @dev Publicly callable.
*/
function depositWithPermit(
ERC20 depositAsset,
uint256 depositAmount,
uint256 minimumMint,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s,
address referralAddress
)
external
virtual
requiresAuth
nonReentrant
revertOnNativeDeposit(address(depositAsset))
returns (uint256 shares)
{
Asset memory asset = _beforeDeposit(depositAsset);

_handlePermit(depositAsset, depositAmount, deadline, v, r, s);

shares = _erc20Deposit(depositAsset, depositAmount, minimumMint, msg.sender, msg.sender, asset);
_afterPublicDeposit(msg.sender, depositAsset, depositAmount, shares, shareLockPeriod, referralAddress);
DepositParams calldata params,
PermitData calldata permit,
address referralAddress,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 deposit natspec missing that params.recipient is bound in the compliance hash

The NatDoc comment correctly notes that native ETH deposits must use the nativeWrapper address in compliance signatures. It should also note that params.recipient (not msg.sender) is bound in the hash, since the compliance signer must explicitly approve the intended recipient:

// The compliance hash covers: (teller, chainId, msg.sender, params.recipient,
// nativeWrapperAddress, depositAmount, deadline) for native ETH deposits.

Comment on lines +116 to +128

// ========================================= BUFFER HELPERS =========================================

/// @notice Execute buffer management before a withdrawal.
function beforeWithdraw(
mapping(ERC20 => BufferHelpers) storage bufferHelpers,
BoringVault vault,
ERC20 withdrawAsset,
uint256 assetAmount
) external {
if (address(bufferHelpers[withdrawAsset].withdrawBufferHelper) != address(0)) {
(address[] memory targets, bytes[] memory data, uint256[] memory values) = bufferHelpers[withdrawAsset].withdrawBufferHelper
.getWithdrawManageCall(address(withdrawAsset), assetAmount);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 processRewards return values from IncentivePool are discarded without logging

TellerWithMultiAssetSupportLib.processRewards calls IncentivePool.processRewards(...) but ignores the returned uint256. Because IncentivePool.processRewards is soft-fail, silent zero-value claims are indistinguishable from successful claims at the teller level. Consider emitting a teller-level event per pool with the returned amount to aid off-chain monitoring and user-facing UX.

* @dev When set to type(uint8).max (255), transfers are unrestricted.
* Any other value restricts transfers so that either `from` or `to` must hold that role.
*/
uint8 public transferAllowedRole = type(uint8).max;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should this not just be handled by the roles auth contract?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is a way to allow the queue to be the allowed operator without adding extra roles.

function allowPermissionedOperator(address operator) external requiresAuth {
beforeTransferData[operator].permissionedOperator = true;
emit AllowPermissionedOperator(operator);
function setTransferAllowedRole(uint8 _transferAllowedRole) external requiresAuth {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can be handled by roles auth depending on the intended flow of who is allowed to call this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the admin/multisig is responsible for defining which role can do transfers, e.g. only addresses that have the BoringQueue role

* Any other value enables checks; the recovered signer must hold that role in the RolesAuthority.
* @param _complianceSignerRole The role ID required for compliance signers, or 255 to disable.
*/
function setComplianceSignerRole(uint8 _complianceSignerRole) external requiresAuth {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same as other role setting function. Depends on intended caller, can be removed or keep if we are not setting this ourselves.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this one should probably be a purely rolesAuth thing tbh. the only consideration that isnt solved by both this and by the rolesAuth route is signer compromise when the rolesAuth has a timelock. but that can be handled by pausing

/**
* @notice Per-user cumulative principal history in base-asset value.
*/
mapping(address user => PrincipalCheckpoint[]) internal _principalHistory;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

group mappings and other like variable types together

assertGt(rB, 0, "bob earns");

_claimReward(alice, rA);
_claimReward(bob, rB);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should we check that rewards earned by bob and alice are equal here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not really, it is a fuzz test and they probably deposit different amounts at the same time, if we are using the same formula for both they shouldn't receive the same amount of rewards. I'll add that bigger deposit = more rewards assert

* When rotating keys, do not reuse a previously active signer address.
* Reusing a retired key would re-enable any unexpired signatures issued under that key.
*/
function setRewardSigner(address newSigner) external requiresAuth {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is good as is so that separate roles can manage separate pools

bxmmm1 and others added 19 commits March 30, 2026 10:15
Merge main into feature/incentive-teller-2, resolving 7 conflicts
in favor of the struct-based deposit API (V0.3) with compliance
signatures introduced on this branch.

Conflict resolution decisions:
- src/base/Roles/TellerWithMultiAssetSupport.sol: kept struct-based
  deposit(DepositParams, address, ComplianceData) over the old
  parameter-based deposit(ERC20, uint256, uint256, address) overload.
  Removed dead _publicDeposit helper from incoming. Version stays V0.3.
- test/TellerVersions.t.sol: kept V0.3 version assertions, dropped
  TellerWithBuffer (no source file exists).
- test/TellerWithMultiAssetSupport.t.sol: kept TransferAllowedRoleSet
  event and COMPLIANCE_SIGNER_ROLE. Re-added deposit-to-receiver and
  refund tests rewritten for struct-based API.
- test/TellerWithMultiAssetSupportBuffer.t.sol: kept buffer helper
  isolation tests from HEAD.
- test/AccountantWithYieldStreaming.t.sol: re-added
  testDepositToInheritsYieldStreamingFlow using struct-based deposit.
- test/TellerWithYieldStreamingBuffer.t.sol: re-added
  testDepositToReceivesSharesAndLocksReceiver and
  testDepositToRefundReturnsAssetsToReceiver using struct-based deposit.
- test/EtherFiLiquid1Migration.t.sol: kept TODO for struct-based
  migration of fork test deposit calls.
- test/integrations/TellerWithReferral.t.sol: accepted deletion
  (intentionally removed on this branch).

Auto-merged fixups (old API leaked through non-conflicting sections):
- Replaced manual keccak256 deposit selectors with
  TellerWithMultiAssetSupport.deposit.selector across all test setUp
  functions.
- Removed ROUTER_ROLE, DEPOSIT_SELECTOR, DEPOSIT_TO_SELECTOR constants
  that referenced the deleted deposit overload.
- Updated fuzzing handlers (TellerHandler.sol) to use struct-based
  deposit, fixed BeforeTransferData destructuring (4 fields not 5),
  added depositSharePrice to DepositRecord and refundDeposit calls,
  removed references to deleted setPermissionedTransfers.
- Updated fuzzing InvariantTestRP/MedusaInvariantTestRP selector arrays
  to remove permissioned transfer selectors.

All 131 tests pass (125 original + 6 re-added deposit-to tests).
Document that depositParams.recipient is ignored in _depositAndBridge
(shares mint to msg.sender before bridging to the `to` param), and that
the compliance hash binds params.recipient as the approved receiver.
Allow authorized callers (e.g. automated bot) to zero out the reward
signer, effectively pausing all claims until a new signer is set.
Update TellerWithMultiAssetSupport deposit flow and cross-chain bridge
to propagate the recipient address. Adjust decoder/sanitizer and update
tests accordingly.
Use TellerWithMultiAssetSupport.deposit.selector instead of hardcoded
keccak256 hash in NonWadScaledVault, NonWadScaledVaultBTC, and
AtomicQueue tests. Update poolInsufficientBalance test to expect
soft no-op after processRewards balance guard was added.
…ction

Merge granular admin setters into single-call alternatives to reduce
bytecode and simplify role configuration:
- denyAll/allowAll/denyFrom/allowFrom/etc -> setDenyFlags
- setComplianceSignerRole + setComplianceWindow -> setComplianceConfig
- setTransferAllowedRole -> setTransferRestrictions (now includes depositForOthersRole)
- stopMessagesFromChain + stopMessagesToChain -> stopMessages

Add depositForOthersRole gate so depositing to a different recipient
can be restricted to authorized addresses. Move deposit-and-bridge
compliance verification into CrossChainTellerLib to reduce leaf
contract bytecode.
…enable

type(uint8).max now means disabled (deposit-for-others blocked) rather than
unrestricted. Any other value enables the feature for holders of that role.
Self-deposits (msg.sender == recipient) are always allowed.
Introduces `principalTeller` state variable and `setPrincipalTeller` admin
function so the queue can call `checkpointQueueWithdrawal` on a teller
each time a withdrawal request is solved, enabling accurate principal
tracking for incentive distribution.
Aligns OnChainWithdraw.nonce struct field, OnChainWithdrawRequested
event param, and local requestNonce variable with the uint88 state
variable. Updates test event signatures and abi.decode calls to match.
* fix: _checkRecipient and beforeTransfer

* docs: natspec

* feat: add sender and recipient to Withdraw event

- Update Withdraw event signature to include indexed user and recipient
- Update emit calls in TellerWithMultiAssetSupport and TellerWithYieldStreaming
- Add expectEmit assertion in PrincipalHistory partial withdraw test

* rename sharePrice -> vaultSharePrice in PrincipalCheckpoint

* safecast in AccountantWithYieldStreaming

* fix: allow withdrawals when transferAllowedRole restricts p2p transfers
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.

3 participants