infiniFi PR 233

Description

MerklRewardsClaimer.claimRewards calls CoreControlled(_farm).emergencyAction to pull rewards into the farm and then immediately transfers IERC20(_rewardToken).balanceOf(_farm) to the recipient. The code never nets out the farm’s pre-existing balance in that token, so any funds already parked on the farm, such as principal or idle liquidity, are swept along with the newly claimed rewards. Because _rewardToken is only checked against enabledRewards, a misconfiguration or malicious reward distribution that happens to use the farm’s asset token will cause the next claim to drain the entire Farm's assets to the recipient, collapsing the reported totalAssets.

Recommendation

Record the farm’s reward token balance before calling emergencyAction, subtract it from the post-claim balance and only transfer the incremental amount. Additionally forbid _rewardToken from equalling the farm’s primary asset unless that behaviour is explicitly intended.

Description

Farm.withdraw records how much value vanished from the farm (assetsSpent = assetsBefore - assetsAfter) but the slippage guard compares the caller supplied amount to the tolerance. Imagine the manager asks to withdraw 100 units, yet the downstream venue only returns 50 due to fees. assetsSpent still reads 100 because the farm’s assets dropped by that figure. minAssetsOut therefore equals ~100, and the guard sees amount >= minAssetsOut (100 >= 100) and happily approves the withdrawal, even though the recipient only got 50. The check never inspects what was actually delivered, so any routing error or fee spike can burn value far beyond maxSlippage while the contract reports success.

Recommendation

Track the recipient’s balance change (or the farm’s assets() delta) and compare that to the tolerance, e.g. require(assetsSpent.mulWadDown(maxSlippage) <= actualAmountDelivered) so the guard enforces what was actually withdrawn, not just the requested amount.

Description

The function assets from contract ERC7540Farm computes the farm’s NAV without accounting for ERC-7540 request states that have become claimable. In ERC-7540 vaults, a depositor’s balance can sit in several states: pending deposit request, pending redeem request, and (after settlement) claimable deposit/redeem. When requestId <= lastDepositEpochIdSettled (or the equivalent for redeems), the view helpers pendingDepositRequest / pendingRedeemRequest return 0, while the value has moved to claimable. As a result, the current logic may omit amounts that are past settlement but not yet claimed, leading to NAV lower than expected and inconsistent reporting immediately before users can deposit/redeem again.

This is particularly relevant for the Plasma fxSave integration the farm targets, but the behavior is inherent to any ERC-7540 vault that follows these state transitions.

Recommendation

Consider to include claimable amounts in the NAV computation alongside any still-pending requests:

• Query both pendingDepositRequest/pendingRedeemRequest and claimableDepositRequest/claimableRedeemRequest for the farm’s address and add all non-zero amounts to NAV.
• Guard for the epoch transition edge case where requestId <= last{Deposit/Redeem}EpochIdSettled: if pending returns 0, check the corresponding claimable* view and include it.
• Document that NAV includes unclaimed-but-claimable balances so reporting stays consistent across settlement boundaries.
• Add unit tests for the boundaries: (a) right before settlement, (b) immediately after settlement (claimable but not claimed), and (c) after claim.
• If gas cost of extra calls is a concern, consider to cache the latest observed requestId and only call claimable* when requestId has crossed the last settled epoch.

Description

ChainlinkOracle.price() still invokes latestAnswer() on the feed contract. Chainlink has deprecated this getter in favour of latestRoundData() (see https://docs.chain.link/data-feeds/api-reference#latestanswer), so the current implementation skips the completeness and freshness checks that Chainlink now exposes through round metadata. If the feed is answered with an outdated or incomplete round, the oracle will still return the stale value and propagate it to accounting, potentially causing mispriced conversions.

Recommendation

Consider switching to the latestRoundData() function.

Description

ERC7540Farm._withdraw forwards funds with a plain ERC20(assetToken).transfer(_to, _amount). Some ERC20s return false on failure and others (notably legacy tokens such as USDT in Ethereum mainnet) return no boolean at all. The current code expects a boolean result and reverts whenever the callee omits the return value, despite the transfer having succeeded. Using SafeERC20.safeTransfer handles both silent success and boolean returns, keeping behaviour aligned with the rest of the codebase.

Recommendation

Replace the manual transfer call with IERC20(assetToken).safeTransfer(_to, _amount) from OpenZeppelin’s SafeERC20, which gracefully handles missing or false return values.

Description

The function price from contract ChainlinkOracle returns the feed value without verifying data freshness against the feed’s heartbeat. Chainlink feeds define a maximum interval between updates; if no update occurs within that interval, the price should be treated as stale. The current logic does not check updatedAt (or any equivalent timestamp) against a configured max age, nor does it provide an alternative failure mode. As a result, downstream consumers may operate using stale prices when the feed stops updating.

Recommendation

Consider to retrieve round data (e.g., via latestRoundData) and enforce freshness before returning:

• Compare updatedAt to a configurable maxDelay aligned with each feed’s heartbeat; if exceeded, revert or emit a StalePrice(feed, updatedAt) event depending on the desired behavior.
• Make maxDelay configurable per feed or per deployment so it can match the official heartbeat.
• Document the chosen failure mode (revert vs. emit-and-continue) and apply it consistently across oracle reads.

Documentation reference (Feeds time official source): https://data.chain.link/feeds

Description

ERC7540Farm.assets() books the raw numbers returned by IERC7540(vault).pendingDepositRequest and pendingRedeemRequest alongside the live share balance. Those getters return the face value that was staged before settlement, but the underlying Vault charges management/performance fees during each settlement via _updateTotalAssetsAndTakeFees. That helper runs _takeFees, minting manager/protocol shares against the same asset base and lowering the post-settlement price per share before pending deposits are converted or pending redeems are cashed out. The farm therefore continues to report the pre-fee figures until the batch is claimed, overstating NAV relative to the amount the vault will actually credit when the requests are finally executed.

Recommendation

Merely informative. We have checked the _PFXSAVE Vault (https://plasmascan.to/address/0x5f264836CE02496CcF55D7F9AA5Cb34e34319DB5/contract/9745/readProxyContract), and the management fee is set to 0 while the performance fee is set to 10%:

[feeRates method Response]managementRate tuple : 0performanceRate : 1000

Description

ERC7540Farm.vaultRequestDeposit calls IERC7540(vault).requestDeposit(_assets, address(this), address(this)). The Hopper Vault implementation insists that the owner be whitelisted before requestDeposit succeeds, so the farm controller must be registered on the vault’s whitelist or deposits revert. If governance forgets to whitelist the farm (or the whitelist is toggled mid-flight), the farm cannot queue deposits even though core code appears configured, leaving funds idle.

Recommendation

Document the dependency and bake it into your deployment checks.

Description

PendleSYFarm.swap accepts _tokenIn and _tokenOut without enforcing that they differ. If the caller supplies the same token, the function still routes through sy.deposit and sy.redeem, triggering superfluous approvals and slippage checks. With the default maxSlippage = 0.999e18, a no-op conversion can still haircut the position by 10 bps because minTokenOut is scaled down before the redemption guard. At best this wastes gas; at worst, integration mistakes could repeatedly erode principal while appearing to “swap” nothing.

Recommendation

Add require(_tokenIn != _tokenOut, InvalidToken(_tokenIn)) (or a dedicated error) at the top of swap so identical assets exit early without incurring Pendle conversions or slippage tolerances.

Description

PendleSYFarm snapshots sy.getTokensIn()/getTokensOut() in the constructor and mirrors those assets into its own allow-list. The Pendle SY implementation is upgradeable and the adapter can change its supported tokens at runtime, so the farm’s whitelist can silently fall out of sync. When Pendle adds or removes tokens, swaps revert as there is no oracle/asset support in the PendleSYFarm.

Recommendation

Run an off-chain monitor that polls sy.getTokensIn() and getTokensOut(), compares the results to the farm’s configured assets and triggers enableAssets/disableAssets plus Accounting oracle updates whenever they diverge.

Description

PendleSYFarm.swap only validates _tokenOut with isAssetSupported, leaving _tokenIn unchecked. Pendle SY will revert on unsupported inputs, so the call is safe, but operators get a Pendle revert instead of a farm-level error. A direct isAssetSupported(_tokenIn) would surface misconfigurations sooner.

Recommendation

Add the same allow-list guard for _tokenIn so unsupported inputs fail fast with a clear farm error rather than bubbling the SY revert.

Description

PendleSYFarm.swap rejects _amountIn == 0 via require(_amountIn > 0, InvalidAmountIn(_amountIn)). Allowing _amountIn == 0 and simply skipping the deposit leg would let callers perform a redemption safely without having to perform any deposit in case that it was needed because, for some reason, the farm had received SY tokens directly.

Recommendation

Remove the strict > 0 requirement and wrap the SY previewDeposit & deposit interactions in an if code block so zero-amount calls bypass the deposit flow:

if(_amountIn > 0){    // swap into SY    IERC20(_tokenIn).forceApprove(address(sy), _amountIn);    uint256 minSyOut = sy.previewDeposit(_tokenIn, _amountIn).mulWadDown(maxSlippage);    sy.deposit{value: 0}(address(this), _tokenIn, _amountIn, minSyOut);}

Description

There are a couple of spelling slips in the farm comments. In PendleSYFarm the header note says “use them to perform covnersions” instead of “conversions”. In AutoFarm the withdraw docstring says “has signficant less slippage”.

Recommendation

Touch up the comments to read “conversions” and “significant” so the documentation looks polished.

Description

The function _withdraw from contract AutoFarm redeems shares directly against the vault with no slippage guardrails:

uint256 share = ERC4626(vault).convertToShares(_amount);ERC4626(vault).redeem(share, _to, address(this));

If the requested withdrawal exceeds the vault’s idle liquidity, the vault may unwind positions and route through DEX liquidity, which can introduce significant slippage (as described in the AutoFarm docs). The current implementation provides no mechanism to bound or mitigate this impact for large withdrawals.

Recommendation

Consider to implement a “large-withdrawal” path similar to the approach outlined in the docs and the Tokemak example, enabling safer execution under specific scenarios:

• Use a staged/streamed unwind or TWAP-like execution to reduce market impact.
• Quote expected proceeds (e.g., via previewRedeem) and enforce user-supplied bounds such as minOut/maxSlippageBps, reverting when unmet.
• Split the withdrawal into tranches with per-tranche slippage limits, or route through an external executor that can batch/cross-route with better price discovery.
• Offer an async/queued withdrawal mode that allows strategies to raise liquidity before final settlement.

References: AutoFarm docs, Tokemak example.

Description

The function constructor from contract AutoFarm sets maxSlippage = 0.9999e18. With 1e18 fixed-point scaling, this equals 0.9999, i.e., an allowed slippage of 1 bp (0.01%). It was mentioned the target was 10 bps (0.10%). A 1 bp tolerance is extremely tight and can make routine withdrawals fail due to minor price movements or fees, causing unnecessary reverts under normal market conditions.

Recommendation

Consider to:

• Set maxSlippage to 0.999e18 to allow 10 bps tolerance if that matches the intended policy.
• Replace the floating multiplier with an explicit bps parameter (e.g., uint256 maxSlippageBps) and compute the multiplier on use to avoid confusion.
• Make maxSlippage configurable (governance/owner with events) to adapt per deployment or vault behavior.
• Add unit tests that cover typical withdrawal paths and verify that expected minor deviations do not revert under the configured tolerance.

Description

The function assets from contract ERC7540Farm calls:

• IERC7540(vault).pendingDepositRequest(pendingDepositRequestId, address(this));
• IERC7540(vault).pendingRedeemRequest(pendingRedeemRequestId, address(this));

without guarding against the case where pendingDepositRequestId == 0 or pendingRedeemRequestId == 0. Different ERC-7540 integrations interpret a zero request id differently (e.g., some return the last pending request, others the first, and some may treat it as an invalid sentinel). Since this behavior is not standardized, invoking the views with a zero id can yield inconsistent results and misreport the farm’s NAV.

Recommendation

Consider to:

• Add an explicit check: if a request id is 0, skip the corresponding pending*Request call or resolve the correct id first via a source of truth (e.g., track the latest created ids in the farm or query vault-provided “current/last request id” views when available).
• Normalize the behavior in code: define and enforce a local convention (e.g., “id 0 means no request”) and document it.
• Where applicable, use dedicated vault getters (e.g., “lastPendingDepositId”, “lastPendingRedeemId”) before querying pending*Request, and only call the view if the resolved id is non-zero.
• Add tests against multiple ERC-7540 vault mocks that emulate differing zero-id semantics to ensure consistent NAV accounting.

Description

The functions handling deposits and redemptions from contract ERC7540Farm call:

• At line 107: IERC20(assetToken).forceApprove(vault, _assets) (deposit path), and
• At line 137: IERC20(share).forceApprove(vault, _shares) (redeem path).

If the ERC-7540 vault functions used by the farm do not actually pull tokens via transferFrom (e.g., they burn shares from msg.sender and/or accept assets already transferred in), these approvals are redundant. Keeping allowances to the vault also increases the attack surface (lingering spend rights if the vault is upgraded/compromised) and adds gas overhead due to forceApprove’s write pattern.

Recommendation

Consider to remove token approvals from both the deposit and redeem paths when the vault API does not require them. If an allowance is strictly necessary for a specific call, consider to:

• Use a “safe spend” pattern: only top-up allowance when insufficient, prefer approve(0) then approve(amount) for non-standard ERC-20s, and reset back to 0 after use.
• Prefer permit/Permit2 when supported to avoid persistent allowances.
• Where possible, transfer assets in first (farm → vault) and call the vault method that consumes msg.sender balance, or use the variant that burns shares without transferFrom(owner) so no allowance is needed.

This reduces gas and avoids maintaining unnecessary standing approvals.

Description

The function vaultDeposit from contract ERC7540Farm allows submitting a deposit request amount _assets, but on Plasma’s fxSave (and, in general, ERC-7540 flows) the subsequent submission step can accept less than the originally requested amount. It can also be called again to continue filling the remaining amount, and calling requestDeposit again will aggregate with previously requested-but-unsubmitted amounts. The same behavior applies to redeem requests.

If the farm logic assumes a 1:1 relationship between requested and submitted amounts, or assumes a single-shot settlement, the internal accounting and NAV can become inconsistent around these edges (e.g., part of the request remains pending/claimable while the farm assumes it is fully settled).

Recommendation

Consider to make the deposit and redeem paths explicitly partial-fill aware:

• Track, per request id, the requested, submitted, and remaining amounts; update internal state on each partial submission rather than assuming full fill.
• When (re)submitting, compute the delta to submit from on-chain views (pending and/or claimable), not from the original requested amount.
• Only advance/close the tracked request id after confirming the remaining amount is zero (fully settled), otherwise keep it open for subsequent submissions.
• Emit events on partial fills (e.g., DepositSubmitted(requestId, submitted, remaining) / RedeemSubmitted(...)) to aid monitoring and retries.
• Add tests covering: (a) partial submission < requested, (b) multiple submissions accumulating to full, (c) re-calling requestDeposit/requestRedeem that aggregates with prior amounts, and (d) the analogous redeem scenarios.
• Document the behavior so integrators know they can submit in tranches and that the farm will reconcile state across multiple submissions.

Description

The function constructor from contract HopperVaultFarm performs a token approval using forceApprove (see line 28), but unlike other sections of the codebase, it does not reset the allowance back to zero after use. Keeping a standing allowance to the vault/adapter increases the attack surface (unexpected spend if the spender is upgraded/compromised) and is inconsistent with the safer pattern used elsewhere in the project where approvals are cleared after the operation.

Recommendation

Consider to:

• Set the allowance back to zero after the operation completes (mirror the pattern used in other files), e.g., call IERC20(token).forceApprove(spender, 0) once the spend is no longer required.
• Only increase allowance immediately before the call that requires it and for the exact amount needed.
• Prefer permit/Permit2 when supported to avoid persistent approvals altogether.
• Add tests that assert post-call allowances are zeroed.

Description

The function syncDeposit from contract HopperVaultFarm integrates with a vault that supports synchronous and ERC-7540-style asynchronous deposits, but it does not expose a way to cancel a pending deposit request during the same epoch. In the ERC-7540 implementation you provided, cancelRequestDeposit() is explicitly designed for same-epoch rollbacks: it refunds the requested assets from the vault’s pendingSilo to the requester if settlement cannot proceed this epoch. Without exposing this path at the farm level, funds can remain stuck pending when:

• The vault becomes paused/closing before settlement.
• External router/liquidity steps needed for settlement are unavailable this epoch.
• An operator/input error (wrong amount/receiver/referral) is detected after requesting.
• Caps/policy changes mid-epoch make the route temporarily non-executable.

Recommendation

Consider to expose a guarded cancel function and reconcile accounting:

• Add cancelVaultDeposit() (e.g., whenNotPaused, onlyCoreRole(CoreRoles.FARM_SWAP_CALLER) or appropriate role) that calls the vault’s cancelRequestDeposit() and emits a DepositCanceled(requestId, refundedAssets) event.
• On cancel, recompute any tracked pending amounts/ids and ensure NAV reflects removal of the pending deposit (i.e., exclude amounts that were in pendingSilo).
• Gate the call so it’s only used within the same epoch (mirror the vault’s revert semantics) and provide clear revert reasons in events/logs for operators.
• Add tests for: (a) cancel in same epoch (success), (b) attempt after epoch boundary (revert), (c) cancel when vault paused mid-epoch (success), (d) cancel after operator input mistake then re-request.

Infinifi: The farm is only intended to do synchronous deposits. Although the team is aware that this path may revert under some scenarios it is not considered necessary to implement a cancellable path given the designed uses cases.

Cantina Managed: Acknowledged by the Infinifi team.

Description

The files declare imports that are never referenced in code:

• AutoFarm.sol:8 — import {CoreRoles} from "@libraries/CoreRoles.sol"; (unused)
• MerklRewardsClaimer.sol:7 — import {FarmRegistry} from "@integrations/FarmRegistry.sol"; (unused)

Leaving unused imports increases maintenance noise, can mask refactor mistakes, and may marginally affect compile time/bytecode metadata. It also contradicts typical linting rules and can hide accidental dependency creep.

Recommendation

Consider to:

• Remove both unused imports.
• Add a lint/check in CI to fail on unused imports (e.g., solhint with no-unused-imports, or Foundry’s forge build -vvv + a simple script that greps compiler warnings).
• As a practice, prefer importing only where symbols are referenced, and prune imports during refactors.

Description

The state variables pendingDepositRequestId and pendingRedeemRequestId from contract ERC7540Farm track the current async requests, but there is no guarded way to manually resynchronize them if an ERC-7540 vault rejects a deposit/redeem or otherwise skips/invalidates a request id. Some async vaults can reject or drop requests outside of the standard, which may leave the farm pointing at a stale/nonexistent id and, in turn, misaccounting pending/claimable amounts or blocking subsequent submissions.

Recommendation

Consider to introduce a narrowly scoped, access-controlled resync path:

• Add an admin/guardian function (e.g., resyncPendingRequestIds(uint256 depositId, uint256 redeemId)) to update pendingDepositRequestId and pendingRedeemRequestId when an async request is rejected or skipped.
• Gate with strong controls: only callable by governance/guardian, optionally behind a timelock or when paused, and emit events (PendingIdsResynced(oldDepositId, newDepositId, oldRedeemId, newRedeemId)).
• Add sanity checks against vault state (e.g., verify the provided ids are ≥ last settled ids, or query that they exist/are pending) and require reconciliation of any associated accounting (e.g., pending vs. claimable deltas).
• Document the operational runbook for when to use this function, and add tests covering rejected/invalidated request scenarios and subsequent successful submissions.

Description

The function <function_name> from contract PendleSYFarm calls:

sy.deposit{value: 0}(address(this), _tokenIn, _amountIn, minSyOut);

Passing an explicit zero msg.value is unnecessary and slightly increases bytecode/gas. It can also be misleading by suggesting ETH is conditionally forwarded when this path is strictly ERC-20 based.

Recommendation

Consider to remove the value specifier and rely on the default zero:

sy.deposit(address(this), _tokenIn, _amountIn, minSyOut);

If a native-ETH deposit path is ever introduced, gate it behind a separate branch that forwards non-zero msg.value with explicit checks and events.

Description

The function swap from contract PendleSYFarm redeems all SY held by the contract:

uint256 syBalance = sy.balanceOf(address(this));sy.redeem(address(this), syBalance, _tokenOut, minTokenOut, false);

The syBalance may include leftovers from previous operations. In that case, the caller’s minTokenOut—which is presumably calibrated for the _amountIn → SY → _tokenOut roundtrip of the current call—no longer matches the redeemed amount. This can result in:

• Redeeming more SY than intended, making the provided minTokenOut too loose for the larger redemption and potentially less profitable than expected; or
• A mismatch where minTokenOut is too tight relative to the actual per-unit rate, causing avoidable reverts.

Recommendation

Consider to redeem only the SY minted by the current operation and align minTokenOut to that amount:

• Delta accounting: Snapshot preSY = sy.balanceOf(address(this)) before deposit, then compute syMinted = sy.balanceOf(address(this)) - preSY and redeem exactly syMinted.
• Return value usage (if available): If sy.deposit(...) returns the syOut amount, use that value directly in the subsequent redeem.
• State hygiene: Alternatively, require sy.balanceOf(address(this)) == 0 at function entry (or sweep/settle leftovers) to avoid mixing flows.
• Bound correctness: Ensure minTokenOut reflects only the SY redeemed in this call (e.g., scale the bound by syMinted if the input is expressed per unit).
• Events/telemetry: Emit events with syMinted and syRedeemed to help off-chain builders set accurate minTokenOut for the actual redeemed amount.

These adjustments keep slippage protections meaningful and prevent cross-contamination between historical dust and the current swap.