[VPD 1241] Certik: Venus Labs - Core Feature Reaudit

[GitGuru7]

Description

Resolves #

Checklist

• I have updated the documentation to account for the changes in the code.
• If I added new functionality, I added tests covering it.
• If I fixed a bug, I added a test preventing this bug from silently reappearing again.
• My contribution follows Venus contribution guidelines.

[github-actions]

Package	Line Rate	Branch Rate	Health
contracts	99%	92%	✔
contracts.interfaces	100%	100%	✔
contracts.lib	100%	100%	✔
contracts.oracles	83%	76%	✔
contracts.oracles.common	97%	92%	✔
Summary	93% (562 / 604)	87% (278 / 320)	✔

[Debugger022]

better to have same validation in setGrowthRate (against the stored snapshot) when interval goes 0 → non-zero,

[GitGuru7]

Yes, that would be better. However, we already have the final audit report, and in practice we wouldn't set the values to 0 anyway. So, I think we can include this change the next time the contract goes through an audit.

[trumpgpt-bot]

test

[trumpgpt-bot]

What changed

Core security fix (Certik DS2-87 + DS2-88) — CorrelatedTokenOracle.setSnapshot()

Two new input guards are added, active when snapshotInterval != 0 (capping is enabled):

• DS2-88 — _snapshotMaxExchangeRate == 0 is now rejected. Previously a zero value would cause getMaxAllowedExchangeRate() to return 0, and getPrice() would fall through to the uncapped path, silently disabling the growth cap.
• DS2-87 — _snapshotTimestamp == 0 || > block.timestamp is now rejected. A future or zero timestamp would make block.timestamp - snapshotTimestamp underflow (Solidity 0.8 reverts), DoS-ing every getPrice() and getMaxAllowedExchangeRate() call.

New InvalidSnapshotTimestamp custom error added. Tests updated to assert the new reverts and to confirm the bypass case (both args zero allowed when capping is disabled via setGrowthRate(0,0)).

Documentation-only changes (no logic):

• ResilientOracle._updateAssetPrice — comment clarifies the empty catch {} is intentional (swallows reverts from non-CorrelatedTokenOracle main oracles)
• OneJumpOracle.INTERMEDIATE_ORACLE — @dev clarifies the oracle must return a token-denominated rate, not USD
• DeviationBoundedOracle NatSpec — improved guidance on when to enable/disable the per-asset transient cache

Audit PDF — Certik reaudit report added.

---

What needs to be updated for mainnet

CorrelatedTokenOracle is abstract and all concrete implementations use a constructor + immutables (no UUPS proxy). All deployed oracle instances must be redeployed to get the new validation. The affected concrete types are the 15 inheritors: OneJumpOracle, ERC4626Oracle, WstETHOracleV2, WeETHOracle, WeETHAccountantOracle, EtherfiAccountantOracle, SlisBNBOracle, BNBxOracle, AsBNBOracle, AnkrBNBOracle, StkBNBOracle, WBETHOracle, SFraxOracle, PendleOracle, ZkETHOracle.

A VIP is then required to:

1. Update each affected oracle slot in ResilientOracle to point to the new deployed addresses
2. Grant ACM permissions for setSnapshot(uint256,uint256), setGrowthRate(uint256,uint256), and setSnapshotGap(uint256) on the new oracle addresses

ResilientOracle itself only had a comment change — no upgrade needed.

---

Constructor parity gap (cannot anchor inline — unchanged code)

setSnapshot() now blocks _snapshotTimestamp > block.timestamp when capping is active, but the constructor (line 96) only checks for zero in InvalidInitialSnapshot:

if ((_initialSnapshotMaxExchangeRate == 0 || _initialSnapshotTimestamp == 0) && _snapshotInterval > 0)
    revert InvalidInitialSnapshot();

A deployer who passes a future _initialSnapshotTimestamp with _snapshotInterval > 0 will deploy an oracle where getMaxAllowedExchangeRate() reverts until that timestamp passes, DoS-ing pricing — exactly the DS2-87 scenario. Suggest adding || _initialSnapshotTimestamp > block.timestamp to the constructor condition (or using the new InvalidSnapshotTimestamp error) for parity with the new setSnapshot guard.

[trumpgpt-bot]

[minor] This guard correctly closes DS2-87 for setSnapshot, but the constructor (line 96) has no equivalent check for _initialSnapshotTimestamp > block.timestamp. A deployer can still deploy with a future initial timestamp and snapshotInterval > 0, which causes getMaxAllowedExchangeRate() (line 246: block.timestamp - snapshotTimestamp) to underflow-revert on every price call until the timestamp passes. Suggest extending the constructor's InvalidInitialSnapshot condition (or adding a separate InvalidSnapshotTimestamp revert) with || _initialSnapshotTimestamp > block.timestamp when _snapshotInterval > 0.