Dev - #162
Merged
Merged
Conversation
…logic - Updated IOrionStrategist to inherit from IERC165 and added setVault function for linking to a vault. - Modified submitIntent to operate on the linked vault. - Introduced error handling for vault linking in ErrorsLib. - Updated KBestTvlWeightedAverage and KBestTvlWeightedAverageInvalid contracts to reflect new interface changes. - Implemented vault linking logic in OrionVault to ensure strategists are correctly associated with their vaults.
- Added comprehensive tests for strategist assignment, including scenarios for EOA, non-ERC165, and ERC165 non-IOrionStrategist contracts. - Enhanced the test suite for vault creation and strategist linking logic.
…t and KBestApy variants
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR refactors strategist-vault interactions to use ERC-165 interface detection and two-step binding (setVault followed by submitIntent), introduces a new APY-weighted strategist with share-price checkpointing, adds cross-rate pricing support via optional quote feeds in ChainlinkPriceAdapter, updates vault redeem batching and buffer tracking in LiquidityOrchestrator, and removes fixture-based orchestrator tests while adding comprehensive strategist linking and strategy-specific test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransparentVault
participant Strategist
participant ERC165Checker
participant Config
User->>TransparentVault: initialize(strategist_)
TransparentVault->>TransparentVault: Set portfolioIntent to 100% strategist
TransparentVault->>TransparentVault: _linkStrategistVault(strategist_)
TransparentVault->>ERC165Checker: Check if strategist<br/>is contract
alt strategist is contract
TransparentVault->>ERC165Checker: IERC165(strategist).supportsInterface<br/>(IOrionStrategist)
alt supports IOrionStrategist
ERC165Checker-->>TransparentVault: true
TransparentVault->>Strategist: setVault(address(this))
Strategist->>Strategist: Store vault reference
Strategist-->>TransparentVault: ✓ Linked
else does not support
ERC165Checker-->>TransparentVault: false
TransparentVault-->>TransparentVault: Skip linking (non-strategist)
end
else not a contract (EOA)
TransparentVault-->>TransparentVault: Skip linking (EOA)
end
TransparentVault-->>User: Initialized
sequenceDiagram
participant Caller
participant KBestStrategist
participant LinkedVault
participant Config
Caller->>KBestStrategist: setVault(address vault_)
KBestStrategist->>KBestStrategist: Validate vault != zero
KBestStrategist->>KBestStrategist: Check not already<br/>linked to different vault
alt already linked to different vault
KBestStrategist-->>Caller: ✗ StrategistVaultAlreadyLinked
else new or same vault
KBestStrategist->>KBestStrategist: _vault = vault_
KBestStrategist-->>Caller: ✓ Stored
end
Caller->>KBestStrategist: submitIntent()
KBestStrategist->>KBestStrategist: Validate _vault != zero
KBestStrategist->>Config: getAllWhitelistedAssets()
Config-->>KBestStrategist: assets[]
KBestStrategist->>KBestStrategist: Calculate positions<br/>(TVL/APY ranking)
KBestStrategist->>KBestStrategist: Compute normalized<br/>weights (sum to 1e9)
KBestStrategist->>LinkedVault: submitIntent(IntentPosition[])
LinkedVault->>LinkedVault: Validate & store intent
LinkedVault-->>KBestStrategist: ✓ Accepted
KBestStrategist-->>Caller: ✓ Submitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/PassiveStrategist.test.ts (2)
300-303:⚠️ Potential issue | 🔴 CriticalInconsistent
submitIntentcall uses old signature.This test still passes
transparentVaultas an argument tosubmitIntent, but the updatedKBestTvlWeightedAveragecontract now uses a parameterlesssubmitIntent(). This will either fail to compile (if TypeChain types are updated) or call a non-existent overload.🐛 Proposed fix
- await expect(passiveStrategist.connect(strategist).submitIntent(transparentVault)).to.be.revertedWithCustomError( + await expect(passiveStrategist.connect(strategist).submitIntent()).to.be.revertedWithCustomError( passiveStrategist, "OrderIntentCannotBeEmpty", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/PassiveStrategist.test.ts` around lines 300 - 303, The test is calling submitIntent with an outdated signature; update the call to use the new parameterless submitIntent() on the connected strategist instance (replace passiveStrategist.connect(strategist).submitIntent(transparentVault) with passiveStrategist.connect(strategist).submitIntent()), and search for any other usages of submitIntent(transparentVault) in the test suite to change them to the parameterless submitIntent() so the assertion using revertedWithCustomError(passiveStrategist, "OrderIntentCannotBeEmpty") still targets the correct call.
333-348:⚠️ Potential issue | 🟠 MajorTest does not call
submitIntent()afterupdateParameters.The loop updates
kviaupdateParametersbut then checksvault.getIntent()without callingsubmitIntent(). The vault's intent won't reflect the newkvalue untilsubmitIntent()is called, so this test is checking stale intent data frombeforeEach.🐛 Proposed fix
it("should maintain valid intent weights after parameter changes", async function () { // Test various k values to ensure weights always sum to 100% for (let k = 1; k <= 4; k++) { await passiveStrategist.connect(strategist).updateParameters(k); + await passiveStrategist.connect(strategist).submitIntent(); const [_tokens, weights] = await transparentVault.getIntent();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/PassiveStrategist.test.ts` around lines 333 - 348, The test updates parameters via passiveStrategist.connect(strategist).updateParameters(k) but then reads stale intent from transparentVault.getIntent() without applying the new parameters; call passiveStrategist.connect(strategist).submitIntent() (or the appropriate submitIntent method on passiveStrategist) after each updateParameters(k) and before transparentVault.getIntent() so the vault reflects the updated intent; ensure you await the submitIntent() call so the subsequent transparentVault.getIntent() returns the new weights and the summed totalWeight check is valid.contracts/strategies/KBestTvlWeightedAverage.sol (1)
131-136:⚠️ Potential issue | 🟡 MinorPotential overflow in weight calculation.
The calculation
uint32((topTvls[i] * intentScale) / totalTVL)can overflow iftopTvls[i] * intentScaleexceedstype(uint256).max. While unlikely with typical TVL values,KBestApyWeightedAverageusesMath.mulDivfor this same calculation, which handles overflow safely.🛡️ Proposed fix using mulDiv
for (uint16 i = 0; i < kActual; ++i) { - uint32 weight = uint32((topTvls[i] * intentScale) / totalTVL); + uint32 weight = uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight: weight }); sumWeights += weight; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/strategies/KBestTvlWeightedAverage.sol` around lines 131 - 136, The weight multiplication can overflow when computing uint32((topTvls[i] * intentScale) / totalTVL); replace the raw multiply/divide with a safe mulDiv call (as used in KBestApyWeightedAverage) to compute weight = uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); update the assignment to intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight: weight }) and keep sumWeights accumulation unchanged; ensure Math.mulDiv is imported/available in this contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/interfaces/IOrionStrategist.sol`:
- Around line 12-17: The setVault(address vault_) function is currently
permissionless and allows third parties to pre-bind a strategist; change it so
only the vault itself can bind the strategist and add a cross-check to ensure
the vault acknowledges this strategist: require vault_ != address(0),
require(msg.sender == vault_) (or otherwise authenticate the caller as the
vault), and call the vault contract (e.g., IVault(vault_).strategist() or
equivalent) to assert it already points to this strategist (or that it expects
this strategist) before storing the vault and emitting the link; keep the
existing StrategistVaultAlreadyLinked behavior for idempotent calls from the
same vault.
In `@contracts/strategies/ApyStrategistBase.sol`:
- Around line 114-129: The NatSpec for _getAssetApy is misleading: the function
calculates a simple annualized return ((P1−P0)/P0 scaled to a year) not a
compounded APY; update the comment above function _getAssetApy to say it returns
a "simple annualized return (non‑compounded) in WAD" and mention it is used as
an APY proxy for ranking, so readers know compounding is not applied. Also keep
the existing behavior and names (_getAssetApy, cp.sharePrice, SECONDS_PER_YEAR,
WAD) unchanged—only adjust the documentation text to reflect “simple annualized
return” semantics.
In `@contracts/strategies/EqualWeight.sol`:
- Around line 43-45: The cast to uint16 for the asset count (uint16 n =
uint16(assets.length)) in EqualWeight.sol risks truncation when assets.length >
65,535; replace the uint16 cast with a uint256 counter (e.g., use uint256 n =
assets.length) or add a defensive require that assets.length <= type(uint16).max
before casting, and keep the existing empty-check revert
(ErrorsLib.OrderIntentCannotBeEmpty()) intact; update any loops or uses of n
(and related index variables) such as in functions referencing
assets/getAllWhitelistedAssets() to use the matching uint256 type.
In `@contracts/strategies/KBestApyEqualWeighted.sol`:
- Around line 27-30: The submitIntent function in KBestApyEqualWeighted is
implementing a declaration from IOrionStrategist (via ApyStrategistBase) but
lacks the required Solidity override specifier; update the function signature
for submitIntent() in KBestApyEqualWeighted to include the override keyword
(e.g., function submitIntent() external override) so it properly overrides the
interface method.
In `@test/NewStrategies.test.ts`:
- Around line 812-837: The test only sets APY state once (inside the hasApy && k
=== 1 branch) but then relies on that state for subsequent iterations, making it
fragile; change the loop so that for every iteration where hasApy is true you
run the APY setup (call strategy.updateCheckpoints(...), advancePastMinWindow(),
and simulateGains(...) each time) or explicitly reset state between iterations
so each k with hasApy=true performs its own checkpointing and gain simulation;
locate and modify the block around strategy.updateCheckpoints,
advancePastMinWindow, and simulateGains in the loop (and/or add a reset/teardown
before each iteration) to ensure APY data is established per-iteration rather
than only when k === 1.
- Around line 40-49: Add explicit assertions before using non-null assertions on
`event` and `parsed` in the helper inside NewStrategies.test.ts: after locating
`event` from `receipt?.logs` (via `factory.interface.parseLog`) assert `event`
is defined (e.g., `expect(event).toBeDefined()` or throw a descriptive error)
and then parse it into `parsed` and assert `parsed` and `parsed.args[0]` are
defined before calling `ethers.getContractAt("OrionTransparentVault",
parsed!.args[0])`; this replaces blind `event!`/`parsed!` usage with clear,
early failures and makes the test error messages informative.
In `@test/StrategistLinking.test.ts`:
- Around line 172-194: Add a test that covers the initialize-time linking path
by creating a fresh KBestTvlWeightedAverage strategy and passing it into
createVault() so the vault is initialized with the strategist (exercise
OrionTransparentVault.initialize()), then deposit TVL as in the existing test
and assert that strategy.connect(user).submitIntent() does not revert and
returns the expected intent (e.g., tokens length equals 2); mirror the setup
used in the current test (mintAndDeposit, underlyingAsset/assets, vault
getIntent) but omit the explicit vault.updateStrategist() call to verify
initialize-time linking works.
---
Outside diff comments:
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Around line 131-136: The weight multiplication can overflow when computing
uint32((topTvls[i] * intentScale) / totalTVL); replace the raw multiply/divide
with a safe mulDiv call (as used in KBestApyWeightedAverage) to compute weight =
uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); update the assignment to
intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight:
weight }) and keep sumWeights accumulation unchanged; ensure Math.mulDiv is
imported/available in this contract.
In `@test/PassiveStrategist.test.ts`:
- Around line 300-303: The test is calling submitIntent with an outdated
signature; update the call to use the new parameterless submitIntent() on the
connected strategist instance (replace
passiveStrategist.connect(strategist).submitIntent(transparentVault) with
passiveStrategist.connect(strategist).submitIntent()), and search for any other
usages of submitIntent(transparentVault) in the test suite to change them to the
parameterless submitIntent() so the assertion using
revertedWithCustomError(passiveStrategist, "OrderIntentCannotBeEmpty") still
targets the correct call.
- Around line 333-348: The test updates parameters via
passiveStrategist.connect(strategist).updateParameters(k) but then reads stale
intent from transparentVault.getIntent() without applying the new parameters;
call passiveStrategist.connect(strategist).submitIntent() (or the appropriate
submitIntent method on passiveStrategist) after each updateParameters(k) and
before transparentVault.getIntent() so the vault reflects the updated intent;
ensure you await the submitIntent() call so the subsequent
transparentVault.getIntent() returns the new weights and the summed totalWeight
check is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 370fa347-8591-4573-b141-cd9d708283aa
📒 Files selected for processing (15)
contracts/interfaces/IOrionStrategist.solcontracts/libraries/ErrorsLib.solcontracts/strategies/ApyStrategistBase.solcontracts/strategies/EqualWeight.solcontracts/strategies/KBestApyEqualWeighted.solcontracts/strategies/KBestApyWeightedAverage.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/test/KBestTvlWeightedAverageInvalid.solcontracts/test/MockERC165NonStrategist.solcontracts/test/MockNonERC165Contract.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.soltest/NewStrategies.test.tstest/PassiveStrategist.test.tstest/StrategistLinking.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/ApyStrategistBase.sol`:
- Around line 52-57: setVault currently allows any caller to set _vault once and
thus can be hijacked; replace this with a two-step, contract-only linking flow:
add proposeVault(address proposed) callable only by the strategist/admin that
sets a new storage variable proposedVault, and change setVault to be callable
only by the proposedVault contract itself (require(msg.sender == proposedVault
&& Address.isContract(msg.sender))) which then sets _vault and clears
proposedVault (and keep the existing zero-address checks and
StrategistVaultAlreadyLinked guard). Update ErrorsLib usage to validate and
revert appropriately when propose/accept flow is misused (e.g., no proposed
vault or already linked).
- Line 52: The function setVault in ApyStrategistBase.sol implements
IOrionStrategist.setVault but is missing the Solidity override specifier; update
the function declaration for setVault to include the override keyword (e.g.,
function setVault(address vault_) external override { ... }) so the compiler
recognizes it as implementing IOrionStrategist.setVault and matches the
interface signature.
- Around line 69-70: The loops in ApyStrategistBase (e.g., where `uint16 n =
uint16(assets.length); for (uint16 i = 0; i < n; ++i)`) silently truncate when
`assets.length` exceeds 65,535; update these to use `uint256` for the length and
loop index (replace `uint16 n` and `uint16 i` with `uint256`) throughout the
file (including the loop ranges at the locations noted and in
`_submitIntentInternal`), or alternatively add an explicit require that reverts
if `assets.length > type(uint16).max` and keep `uint16` if you must; also ensure
upstream `OrionConfig.getAllWhitelistedAssets()` does not cast to `uint16` (or
adds the same guard) so the length is consistent end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02f6fd32-ae2d-4dcc-ad09-834880e620ef
📒 Files selected for processing (1)
contracts/strategies/ApyStrategistBase.sol
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/EqualWeight.sol`:
- Around line 31-36: The function setVault in EqualWeight implements
IOrionStrategist but lacks the required Solidity override specifier; update the
function signature to include override (e.g., change "function setVault(address
vault_) external {" to "function setVault(address vault_) external override {"
or "external override(IOrionStrategist)" if there are multiple inherited
interfaces) so the compiler recognizes this as the interface implementation
while leaving the body logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d3a9437-0155-4e15-bd7a-1b7c3f2ea7f1
📒 Files selected for processing (7)
contracts/strategies/ApyStrategistBase.solcontracts/strategies/EqualWeight.solcontracts/strategies/KBestApyEqualWeighted.solcontracts/strategies/KBestApyWeightedAverage.solcontracts/test/MockNoDecimalsAsset.soltest/NewStrategies.test.tstest/StrategistLinking.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/KBestApyWeightedAverage.sol`:
- Around line 25-27: Reject zero selections and guard before dividing/indexing:
validate and reject k == 0 in the constructor (KBestApyWeightedAverage) and in
updateParameters so the stored k cannot be set to 0; additionally, in the method
that computes kActual/intentScale/intent (the selection/weighting flow where
variables kActual, intentScale and intent are used) add a require/revert if
kActual == 0 before performing intentScale / kActual or indexing intent[0] to
avoid panics from division-by-zero or empty-array access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92254ced-e1c9-4ae9-9f34-8904f4c271e5
📒 Files selected for processing (1)
contracts/strategies/KBestApyWeightedAverage.sol
Supports an optional quote feed for cross-rate normalisation (e.g. ETH/USD / USDC/USD to obtain ETH/USDC)
…buffer state after partial minibatch failures.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/PassiveStrategist.test.ts (1)
300-303:⚠️ Potential issue | 🔴 CriticalInconsistent API usage:
submitIntentis called with an argument but the interface is now parameterless.Line 300 passes
transparentVaultas an argument tosubmitIntent(), but theIOrionStrategistinterface now definessubmitIntent()as parameterless. This will cause a compilation or runtime error.🐛 Proposed fix
- await expect(passiveStrategist.connect(strategist).submitIntent(transparentVault)).to.be.revertedWithCustomError( + await expect(passiveStrategist.connect(strategist).submitIntent()).to.be.revertedWithCustomError(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/PassiveStrategist.test.ts` around lines 300 - 303, The test calls submitIntent(transparentVault) but the IOrionStrategist API changed to a parameterless submitIntent(); update the test to call submitIntent() with no arguments (remove transparentVault), and ensure any related expectations still reference passiveStrategist.connect(strategist).submitIntent(); confirm the test uses the correct signer/context (connect(strategist)) and that no other tests pass parameters to submitIntent anywhere else.contracts/strategies/KBestTvlWeightedAverage.sol (1)
88-110: 🧹 Nitpick | 🔵 TrivialConsider using sentinel pattern for consistency with
KBestApyStrategist.
_selectTopKAssetsinitializesmaxIndex = 0, whileKBestApyStrategist._selectTopKByApyusesmaxIndex = type(uint16).maxas a sentinel. The current approach works (defaulting to index 0 when all TVLs are equal), but using the sentinel pattern consistently across strategists would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/strategies/KBestTvlWeightedAverage.sol` around lines 88 - 110, In _selectTopKAssets, replace the current defaulting behavior by using the sentinel pattern like KBestApyStrategist: initialize maxIndex as type(uint16).max inside the outer loop, update maxIndex when a new maxTVL is found, and after the inner loop ensure maxIndex != type(uint16).max before marking used and writing tokens/topTvls (or break if sentinel remains); this makes selection consistent with KBestApyStrategist and avoids implicit defaulting to index 0.
♻️ Duplicate comments (3)
test/NewStrategies.test.ts (1)
34-53: 🧹 Nitpick | 🔵 TrivialAdd explicit guard before non-null assertions in
createVaulthelper.The helper uses
event!andparsed!which will throw unclear errors if the event isn't found. The past review suggested adding explicit checks.♻️ Suggested improvement
const event = receipt?.logs.find((log) => { try { const parsed = factory.interface.parseLog(log); return parsed?.name === "OrionVaultCreated"; } catch { return false; } }); + if (!event) throw new Error("OrionVaultCreated event not found in transaction receipt"); const parsed = factory.interface.parseLog(event!); + if (!parsed) throw new Error("Failed to parse OrionVaultCreated event"); return ethers.getContractAt("OrionTransparentVault", parsed!.args[0]) as unknown as Promise<OrionTransparentVault>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NewStrategies.test.ts` around lines 34 - 53, The createVault helper currently uses non-null assertions on event and parsed which can produce unclear runtime errors; modify createVault to explicitly check that the receipt logs contain the "OrionVaultCreated" event (after calling factory.interface.parseLog) and throw a clear, descriptive Error if event is undefined or parsing fails (include strategistAddr or tx hash for context), and also validate parsed.args[0] exists before calling ethers.getContractAt("OrionTransparentVault", ...); update the code paths that reference event and parsed to use these guards instead of event! and parsed!.test/StrategistLinking.test.ts (1)
24-43: 🧹 Nitpick | 🔵 TrivialSame
createVaulthelper pattern needs explicit guards.This helper duplicates the pattern from
NewStrategies.test.tswith the same non-null assertion concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/StrategistLinking.test.ts` around lines 24 - 43, The createVault helper uses non-null assertions for receipt, event and parsed which can crash tests; add explicit guards and clear errors: after awaiting tx.wait() verify receipt is defined, ensure receipt.logs contains a log that parses to an "OrionVaultCreated" event (handle parse failures with try/catch), assert the found event and parsed object are not undefined and that parsed.args[0] exists before calling factory.interface.parseLog(event) and ethers.getContractAt; if any check fails, throw a descriptive error so failures are deterministic and easy to debug (update symbols: createVault, receipt, event, parsed, factory.interface.parseLog, parsed.args[0]).contracts/strategies/KBestApyStrategist.sol (1)
82-87:⚠️ Potential issue | 🔴 Critical
setVaultremains vulnerable to front-running hijack.The past review flagged that any caller can permanently bind
_vaultto an arbitrary address before the legitimate vault callssetVault. This concern persists: if an attacker front-runs the vault creation/linking transaction, they can set_vaultto their own contract, causing all futuresubmitIntent()calls to target the attacker's address.The recommended mitigation is to restrict
setVaultto an authorized flow (e.g., require the caller to be a registered vault inOrionConfig, or implement a propose/accept pattern).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/strategies/KBestApyStrategist.sol` around lines 82 - 87, The setVault function currently allows any caller to permanently set _vault, enabling front-running hijacks; modify setVault to restrict who can set/override _vault by either (A) requiring the caller be an authorized vault from OrionConfig (e.g., check OrionConfig.isRegisteredVault(msg.sender) or similar) before assigning _vault, or (B) implement a two-step propose/accept flow for linking a vault: add proposeVault(address candidate) that stores a pendingVault and an acceptVault() callable only by that candidate (or the current legitimate vault) to finalize _vault, and ensure submitIntent() continues to reference _vault; update associated error/revert conditions (ErrorsLib.StrategistVaultAlreadyLinked(), ZeroAddress) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Around line 40-45: The setVault function in KBestTvlWeightedAverage.sol allows
any caller to bind _vault and can be front-run; restrict who can call or
implement a propose/accept flow. Either add an access control check (e.g.,
require(msg.sender == owner() or a vault registry check) so only an authorized
actor can call setVault, or replace setVault with a two-step pattern
(proposeVault(address) callable by the legitimate vault/owner and acceptVault()
callable by the proposed address) to avoid third-party hijack; update related
logic and events accordingly and mirror the same protection used in
KBestApyStrategist.setVault to keep strategist implementations consistent.
- Around line 48-62: Add ReentrancyGuard to KBestTvlWeightedAverage and mark
submitIntent as nonReentrant: import OpenZeppelin's ReentrancyGuard, have the
contract inherit from ReentrancyGuard, and add the nonReentrant modifier to the
submitIntent function (which performs the external call
IOrionTransparentVault(vault_).submitIntent). Ensure any existing inheritance
order is updated accordingly and recompile to confirm no constructor changes are
required.
In `@contracts/test/MockChainlinkFeed.sol`:
- Around line 28-38: The mock contract MockChainlinkFeed currently returns
block.timestamp for startedAt in latestRoundData and getRoundData, preventing
tests of the startedAt > block.timestamp path; add a uint256 storage variable
(e.g., _startedAt), add a public setter function setStartedAt(uint256 startedAt)
to set it, and change latestRoundData and getRoundData to return _startedAt
instead of block.timestamp so tests can control startedAt values.
In `@test/ChainlinkPriceAdapterUnit.test.ts`:
- Around line 77-93: The test passes minPrice=0 to adapter.configureFeed when
setting up the feed for asset; update the call to adapter.configureFeed in the
test "scaleFactor correct when base=18dec, quote=8dec → 10^8" to use a non-zero
minPrice (e.g., 1) instead of 0 so the test remains realistic and consistent
with other tests that enforce a minimum price; keep the same parameters
otherwise (asset, await base18.getAddress(), false, STALENESS, <minPrice>,
MAX_PRICE, await quoteFeed.getAddress()) and verify cfg.scaleFactor as before.
In `@test/orchestrator/Orchestrators.test.ts`:
- Around line 21-22: Re-add a lightweight local full-cycle smoke test in
Orchestrators.test.ts that exercises performUpkeep end-to-end and asserts the
core orchestrator invariants: call the same performUpkeep flow used previously,
then verify LiquidityOrchestrator returns to Idle, buffer/accounting balances
are consistent, and epoch-based liquidity/deposit effects occurred; locate the
test harness that calls performUpkeep and the LiquidityOrchestrator instance in
the file and reintroduce a short test (e.g., "full cycle smoke test") that
performs the upkeep, advances the epoch, and asserts those invariants so the
repo retains direct regression coverage for orchestrator/vault flows.
---
Outside diff comments:
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Around line 88-110: In _selectTopKAssets, replace the current defaulting
behavior by using the sentinel pattern like KBestApyStrategist: initialize
maxIndex as type(uint16).max inside the outer loop, update maxIndex when a new
maxTVL is found, and after the inner loop ensure maxIndex != type(uint16).max
before marking used and writing tokens/topTvls (or break if sentinel remains);
this makes selection consistent with KBestApyStrategist and avoids implicit
defaulting to index 0.
In `@test/PassiveStrategist.test.ts`:
- Around line 300-303: The test calls submitIntent(transparentVault) but the
IOrionStrategist API changed to a parameterless submitIntent(); update the test
to call submitIntent() with no arguments (remove transparentVault), and ensure
any related expectations still reference
passiveStrategist.connect(strategist).submitIntent(); confirm the test uses the
correct signer/context (connect(strategist)) and that no other tests pass
parameters to submitIntent anywhere else.
---
Duplicate comments:
In `@contracts/strategies/KBestApyStrategist.sol`:
- Around line 82-87: The setVault function currently allows any caller to
permanently set _vault, enabling front-running hijacks; modify setVault to
restrict who can set/override _vault by either (A) requiring the caller be an
authorized vault from OrionConfig (e.g., check
OrionConfig.isRegisteredVault(msg.sender) or similar) before assigning _vault,
or (B) implement a two-step propose/accept flow for linking a vault: add
proposeVault(address candidate) that stores a pendingVault and an acceptVault()
callable only by that candidate (or the current legitimate vault) to finalize
_vault, and ensure submitIntent() continues to reference _vault; update
associated error/revert conditions (ErrorsLib.StrategistVaultAlreadyLinked(),
ZeroAddress) accordingly.
In `@test/NewStrategies.test.ts`:
- Around line 34-53: The createVault helper currently uses non-null assertions
on event and parsed which can produce unclear runtime errors; modify createVault
to explicitly check that the receipt logs contain the "OrionVaultCreated" event
(after calling factory.interface.parseLog) and throw a clear, descriptive Error
if event is undefined or parsing fails (include strategistAddr or tx hash for
context), and also validate parsed.args[0] exists before calling
ethers.getContractAt("OrionTransparentVault", ...); update the code paths that
reference event and parsed to use these guards instead of event! and parsed!.
In `@test/StrategistLinking.test.ts`:
- Around line 24-43: The createVault helper uses non-null assertions for
receipt, event and parsed which can crash tests; add explicit guards and clear
errors: after awaiting tx.wait() verify receipt is defined, ensure receipt.logs
contains a log that parses to an "OrionVaultCreated" event (handle parse
failures with try/catch), assert the found event and parsed object are not
undefined and that parsed.args[0] exists before calling
factory.interface.parseLog(event) and ethers.getContractAt; if any check fails,
throw a descriptive error so failures are deterministic and easy to debug
(update symbols: createVault, receipt, event, parsed,
factory.interface.parseLog, parsed.args[0]).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f339606-e4ca-4deb-a46c-548f8a127fec
📒 Files selected for processing (45)
.prettierignoreREADME.mdcontracts/LiquidityOrchestrator.solcontracts/interfaces/ILiquidityOrchestrator.solcontracts/interfaces/IOrionStrategist.solcontracts/interfaces/IOrionVault.solcontracts/libraries/ErrorsLib.solcontracts/price/ChainlinkPriceAdapter.solcontracts/strategies/KBestApyStrategist.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/test/MockChainlinkFeed.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.solpackage.jsontest/ChainlinkPriceAdapterUnit.test.tstest/NewStrategies.test.tstest/PassiveStrategist.test.tstest/RedeemBeforeDepositOrder.test.tstest/Removal.test.tstest/StrategistLinking.test.tstest/VerifyPerformDataRejection.test.tstest/crossAsset/ChainlinkPriceAdapter.test.tstest/crossAsset/ERC4626ExecutionAdapter.test.tstest/crossAsset/ERC4626PriceAdapter.test.tstest/fixtures/Orchestrator1.jsontest/fixtures/Orchestrator2.jsontest/fixtures/Orchestrator3.jsontest/fixtures/Orchestrator4.jsontest/fixtures/Orchestrator5.jsontest/fixtures/Orchestrator6.jsontest/fixtures/RedeemBeforeDepositOrder1.jsontest/fixtures/RedeemBeforeDepositOrder2.jsontest/fixtures/RedeemBeforeDepositOrder3.jsontest/fixtures/RedeemBeforeDepositOrder4.jsontest/fixtures/RedeemBeforeDepositOrder5.jsontest/fixtures/Removal1.jsontest/fixtures/Removal2.jsontest/fixtures/Removal3.jsontest/fixtures/Removal4.jsontest/fixtures/Removal5.jsontest/fixtures/Removal6.jsontest/fixtures/Removal7.jsontest/fixtures/Removal8.jsontest/helpers/orchestratorHelpers.tstest/orchestrator/Orchestrators.test.ts
💤 Files with no reviewable changes (24)
- .prettierignore
- test/fixtures/Removal2.json
- test/fixtures/RedeemBeforeDepositOrder5.json
- test/fixtures/Orchestrator2.json
- test/fixtures/Removal7.json
- test/fixtures/Removal1.json
- test/fixtures/Removal8.json
- test/fixtures/Orchestrator1.json
- test/fixtures/Removal6.json
- test/fixtures/Removal4.json
- test/fixtures/RedeemBeforeDepositOrder3.json
- test/fixtures/Removal3.json
- test/fixtures/Orchestrator3.json
- test/fixtures/Removal5.json
- test/fixtures/Orchestrator5.json
- test/fixtures/RedeemBeforeDepositOrder1.json
- test/fixtures/RedeemBeforeDepositOrder2.json
- test/fixtures/Orchestrator6.json
- test/fixtures/Orchestrator4.json
- test/Removal.test.ts
- test/RedeemBeforeDepositOrder.test.ts
- test/helpers/orchestratorHelpers.ts
- test/VerifyPerformDataRejection.test.ts
- test/fixtures/RedeemBeforeDepositOrder4.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes (v2.2.0)
New Features
pendingRedeemBatchhelper for redemption visibility.Improvements
Bug Fixes