[VPD-808] add internalCash to prevent donation attack#551
Conversation
Replace `balanceOf(address(this))` with an internally tracked `internalCash` variable in `_getCashPrior()` to make exchange rate, borrow, redeem, and interest rate calculations immune to direct token transfers (donation attacks). Contract changes: - Add `internalCash` storage variable to VTokenStorage (gap 48→47) - `_doTransferIn`: increment `internalCash` by actual amount received - `_doTransferOut`: decrement `internalCash` before transfer - `_getCashPrior`: return `internalCash` instead of `balanceOf` - Add ACM-gated `syncCash()` for one-time initialization after upgrade - Add `CashSynced` event and `syncCash()` to VTokenInterface Test changes: - Add `harnessSetInternalCash()` to VTokenHarness - Sync `internalCash` in existing borrow, redeem, liquidate, reserves tests - Add unit tests: syncCash, donation immunity, internalCash tracking - Add fork tests: storage layout preservation, donation attack prevention
- Rewrite donationAttack.ts to dynamically test all markets via comptroller.getAllMarkets() with before/after upgrade sections proving vulnerability exists and is fully blocked - Add normal operations tests (mint, redeem, borrow, repay) on fork verifying internalCash matches actual balanceOf after each operation - Use block 86731940 (same as core pool) for consistent cross-repo test coverage - Add BSC hardfork history to hardhat.config.ts to fix fork test failure at exact block number - Fix BigNumber scientific notation, add zero-balance syncCash unit test, and remove unused imports from internalCashUpgrade.ts
Document the donation attack vulnerability, the THE token incident (BSC Core Pool, March 15 2026), and the internalCash fix. Covers both test suites (DonationAttack + vTokenStorageChecks) across 7 chains.
| * @custom:event Emits CashSynced | ||
| * @custom:access Controlled by AccessControlManager | ||
| */ | ||
| function syncCash() external override nonReentrant { |
There was a problem hiding this comment.
why we dont have sweep logic here ? is there any alternative function for that ?
There was a problem hiding this comment.
in isolated pool sweep function already exist
isolated-pools/contracts/VToken.sol
Line 691 in 3a5afbc
There was a problem hiding this comment.
but this one you cant sweep underlying token ?
require(address(token) != underlying, "VToken::sweepToken: can not sweep underlying token");
There was a problem hiding this comment.
but this one you cant sweep underlying token ?
require(address(token) != underlying, "VToken::sweepToken: can not sweep underlying token");
As discussed, this is no longer needed since we'll be syncing getCash with the sync function.
|
@claude review |
|
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage. Once credits are available, comment |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core contract logic looks correct, but this PR modifies security-critical cash accounting in VToken.sol and introduces a new storage variable with a migration path (syncCash) — it warrants human review before merging.
Extended reasoning...
Overview
This PR replaces IERC20.balanceOf(address(this)) with an internally tracked internalCash variable in _getCashPrior() to prevent donation attacks on VToken markets. The contract changes are small (4 modified functions + 1 new function in VToken.sol, 1 new storage variable in VTokenInterfaces.sol) but affect the core cash accounting used for exchange rate, borrow, redeem, and interest rate calculations. The bulk of the PR (22 of 26 files) is comprehensive test coverage across 7 chains.
Security risks
The implementation itself follows the standard pattern for donation attack prevention (internal balance tracking). However:
- Any bug in
internalCashbookkeeping could cause funds to become locked (ifinternalCashundershoots actual balance) or enable withdrawals beyond what the contract holds (ifinternalCashovershoots). The_doTransferOutdecrement-before-transfer pattern is correct, and_doTransferInuses the before/after balance delta, which handles fee-on-transfer tokens. syncCash()is ACM-gated and usesnonReentrant, which is appropriate. However, it can be called multiple times (not just once), meaning an authorized caller could re-sync at any time, potentially absorbing donated tokens into the protocol accounting. Whether this is intentional or should be one-shot is a design decision for a human reviewer.- The storage gap change (48→47) needs human verification against all deployed proxy contracts to ensure no slot collision.
Level of scrutiny
This requires thorough human review. The contract changes are in production-critical DeFi lending code that manages real user funds. While the diff is small, the blast radius of a bug in cash accounting is severe — it could affect exchange rates, borrow limits, and redemptions across all markets on all chains. The upgrade/migration path (beacon upgrade + per-market syncCash calls) also needs operational review.
Other factors
- There is an active discussion between fred-venus and Debugger022 about whether
sweepTokenshould be updated to allow sweeping underlying tokens (sincesyncCashcan now absorb donated tokens). This design question should be resolved by humans. - Test coverage is thorough, with both unit tests and fork tests across 7 chains verifying storage layout preservation, donation immunity, and normal operations.
- The only automated bug found is a minor NatSpec documentation issue (stale comments on
_getCashPrior), which has no functional impact.
- Fix _getCashPrior() NatSpec to reflect internalCash instead of balanceOf - Fix getCash() NatSpec to clarify it returns internally tracked cash - Fix @param order in _liquidateBorrow() to match function signature - Fix @param order in _liquidateBorrowFresh() to match function signature
- Extract shared test logic into testRunner.ts for both DonationAttack and vTokenStorageChecks - Rewrite all 7 network files as thin configs (~12 lines each), reducing ~8000 lines to ~1200 - Move shared helpers (getCoreComptroller, getComptrollerAdmin, getBalanceSlot, getBeaconOwner, setTokenBalance) into DonationAttack/helpers.ts - Resolve comptroller, admin, ACM, and beacon addresses dynamically instead of hardcoding - Fix incorrect VToken constructor args in vTokenStorageChecks (was using ethereum values for all chains) - Add hardfork history for Optimism, Unichain, Arbitrum in hardhat.config.ts - Add decimals() try/catch guard to handle non-standard proxy tokens (e.g. vweETH_Core on Unichain)
| */ | ||
| function _getCashPrior() internal view virtual returns (uint256) { | ||
| return IERC20Upgradeable(underlying).balanceOf(address(this)); | ||
| return internalCash; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🔴 Shortfall.closeAuction() transfers recovered tokens directly to the vToken via erc20.safeTransfer() (Shortfall.sol:244), bypassing doTransferIn(), so internalCash is never incremented. When badDebtRecovered() (VToken.sol:655) then reduces badDebt, the exchange rate formula (internalCash + totalBorrows + badDebt - totalReserves) / totalSupply loses the offset -- badDebt drops but internalCash stays the same -- causing the exchange rate to incorrectly decrease on every bad debt recovery, resulting in value loss for all vToken holders. Fix: add internalCash += recoveredAmount to badDebtRecovered().
Extended reasoning...
What the bug is
The badDebtRecovered() function at VToken.sol:655-664 is called by Shortfall.closeAuction() after a bad debt auction completes. It reduces the badDebt state variable but does not update internalCash. Before this PR, _getCashPrior() returned IERC20.balanceOf(address(this)), which automatically reflected the tokens sent via erc20.safeTransfer() in Shortfall.sol:244. After this PR, _getCashPrior() returns the internally tracked internalCash variable, which is only updated through _doTransferIn() and _doTransferOut().
The specific code path
- Shortfall.closeAuction() at line 244:
erc20.safeTransfer(address(auction.markets[i]), auction.bidAmount[auction.markets[i]])-- sends underlying tokens directly to the vToken contract. This is a plain ERC20 transfer that bypasses _doTransferIn(), so internalCash is NOT incremented. - Shortfall.closeAuction() at line 248:
auction.markets[i].badDebtRecovered(marketsDebt[i])-- calls VToken.badDebtRecovered() which reduces badDebt by the recovered amount but does not touch internalCash. - The exchange rate in _exchangeRateStored() is computed as
(internalCash + totalBorrows + badDebt - totalReserves) / totalSupply. Since badDebt decreased but internalCash did not increase, the numerator shrinks, causing the exchange rate to drop.
Why existing code doesn't prevent it
The badDebtRecovered() function was written before this PR and only modifies badDebt. It has no knowledge of internalCash. The Shortfall contract is an external contract that uses direct ERC20 transfers (not _doTransferIn). This PR modified _doTransferIn, _doTransferOut, and _getCashPrior in VToken but did not update badDebtRecovered() to account for the new internalCash tracking.
Step-by-step proof
- Market state: internalCash = 1000, totalBorrows = 2000, badDebt = 500, totalReserves = 100, totalSupply = 100
- Exchange rate = (1000 + 2000 + 500 - 100) / 100 = 34
- A Shortfall auction closes, recovering 500 tokens of bad debt
- Shortfall.sol:244 transfers 500 tokens directly to vToken. Actual balanceOf increases to 1500, but internalCash stays at 1000
- Shortfall.sol:248 calls badDebtRecovered(500). badDebt becomes 0, internalCash stays at 1000
- Exchange rate = (1000 + 2000 + 0 - 100) / 100 = 29
- Exchange rate dropped from 34 to 29, a ~14.7% loss for all depositors
Impact
Every bad debt recovery auction will incorrectly reduce the exchange rate, causing real value loss for all vToken holders. An attacker could exploit the predictable exchange rate drop by redeeming before an auction close and minting after at the lower rate, then waiting for syncCash() to restore the correct rate and redeeming at a profit.
Fix
Add internalCash += recoveredAmount_ to the badDebtRecovered() function in VToken.sol, so the increase in tracked cash offsets the decrease in bad debt, keeping the exchange rate stable.
There was a problem hiding this comment.
just this finding worth more than $100, loll
| ```bash | ||
| # Donation attack tests | ||
| FORK=ethereum npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/ethereum.ts | ||
| FORK=arbitrumone npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/arbitrumone.ts | ||
|
|
||
| # Storage checks | ||
| FORK=ethereum npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/ethereum.ts | ||
| FORK=arbitrumone npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/arbitrumone.ts |
There was a problem hiding this comment.
🟡 README fork test commands (lines 96-101) use FORK=ethereum but the test files check process.env.FORK === "true" and process.env.FORKED_NETWORK === FORKED_NETWORK, so tests silently skip with 0 tests passed. The correct command should be FORK=true FORKED_NETWORK=ethereum HARDHAT_FORK_NETWORK=ethereum npx hardhat test .... A developer following the README would see mocha pass with 0 tests and incorrectly believe all tests succeeded.
Extended reasoning...
What the bug is
The README at tests/hardhat/Fork/ExchangeRateManipulation/README.md (lines 94-101) documents fork test commands in the format:
FORK=ethereum npx hardhat test tests/hardhat/Fork/.../ethereum.tsHowever, every test file (e.g., DonationAttack/ethereum.ts:3,6) uses this guard pattern:
const FORK = process.env.FORK === "true";
const FORKED_NETWORK = "ethereum";
if (FORK && process.env.FORKED_NETWORK === FORKED_NETWORK) {
runDonationAttackTests({ ... });
}How it manifests
When a developer runs FORK=ethereum npx hardhat test ..., the environment variable FORK is set to the string "ethereum". The test checks process.env.FORK === "true", which evaluates to false because "ethereum" !== "true". This means the if block is never entered, runDonationAttackTests() is never called, and mocha sees zero test cases. Mocha passes with 0 tests by default, so the developer sees a green result and believes the tests passed.
Step-by-step proof
- Developer reads README line 96 and runs:
FORK=ethereum npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/ethereum.ts - Node process starts with
process.env.FORK = "ethereum" ethereum.tsline 3 evaluates:const FORK = ("ethereum" === "true")→false- Line 6:
if (false && ...)→ entire block skipped - No
describe/itblocks are registered with mocha - Mocha reports:
0 passing— exit code 0 (success) - Developer incorrectly concludes tests pass
Additionally, even if FORK were correct, the README omits the required FORKED_NETWORK env var (checked on line 6 of each test file) and HARDHAT_FORK_NETWORK (used by hardhat.config.ts lines 275/309-311 to enable forking and load deployment artifacts). Without HARDHAT_FORK_NETWORK, no fork is created and deployments are not loaded.
Impact
This is a documentation-only issue — no contract code or production behavior is affected. However, it means the fork tests added by this PR to validate the internalCash donation attack fix cannot actually be run by any developer or reviewer following the README instructions. The tests would silently not execute, giving a false sense of confidence that the fix has been validated across all 7 chains.
How to fix
Replace the README commands (lines 96-101) with the correct format:
# Donation attack tests
FORK=true FORKED_NETWORK=ethereum HARDHAT_FORK_NETWORK=ethereum npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/ethereum.ts
FORK=true FORKED_NETWORK=arbitrumone HARDHAT_FORK_NETWORK=arbitrumone npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/arbitrumone.ts
# Storage checks
FORK=true FORKED_NETWORK=ethereum HARDHAT_FORK_NETWORK=ethereum npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/ethereum.ts
FORK=true FORKED_NETWORK=arbitrumone HARDHAT_FORK_NETWORK=arbitrumone npx hardhat test tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/arbitrumone.tsNote: bug_019 and bug_021 describe the exact same issue and have been merged here.
Add deploy/023-market-upgrade.ts for deploying new VToken implementations without skipIfAlreadyDeployed. Add Base mainnet/sepolia to etherscan custom chains and enable etherscan verification explicitly.
The badDebtRecovered function was not updating internalCash after recovering bad debt from auction, causing the internal cash balance to drift from the actual underlying token balance.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds on-chain Changes
Sequence DiagramsequenceDiagram
participant Admin
participant BeaconOwner as Beacon Owner
participant VTokenBeacon
participant NewImpl as VTokenImpl (new)
participant ACM as AccessControlManager
participant Market as VToken Market
participant User
Admin->>NewImpl: deploy new implementation
Admin->>BeaconOwner: verify beacon implementation
BeaconOwner->>VTokenBeacon: upgradeTo(NewImpl)
VTokenBeacon-->>NewImpl: new implementation active
Admin->>ACM: grant syncCash role to actor
ACM-->>Admin: role granted
Admin->>Market: call syncCash()
Market->>Market: read underlying.balanceOf(address(this))
Market->>Market: set internalCash = actualBalance
Market-->>Admin: emit CashSynced(oldInternalCash, newInternalCash)
User->>Market: mint / redeem / borrow / repay
Market->>Market: _doTransferIn increments internalCash
Market->>Market: _doTransferOut decrements internalCash
Market->>Market: _getCashPrior returns internalCash
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/VToken.sol (1)
655-663:⚠️ Potential issue | 🟠 MajorValidate recovered cash before crediting
internalCash.This path trusts
recoveredAmount_and bumpsinternalCashwithout checking the real post-transfer balance. If the recovery flow overstates the amount or the underlying takes a fee,internalCashcan end up abovebalanceOf(address(this)), and borrows/redeems start using stale cash again.🛡️ Possible hardening
function badDebtRecovered(uint256 recoveredAmount_) external { require(msg.sender == shortfall, "only shortfall contract can update bad debt"); require(recoveredAmount_ <= badDebt, "more than bad debt recovered from auction"); + uint256 newInternalCash = internalCash + recoveredAmount_; + require( + IERC20Upgradeable(underlying).balanceOf(address(this)) >= newInternalCash, + "recovered cash not received" + ); uint256 badDebtOld = badDebt; uint256 badDebtNew = badDebtOld - recoveredAmount_; badDebt = badDebtNew; - internalCash += recoveredAmount_; + internalCash = newInternalCash; emit BadDebtRecovered(badDebtOld, badDebtNew); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/VToken.sol` around lines 655 - 663, The badDebtRecovered function trusts recoveredAmount_ and immediately increases internalCash and reduces badDebt; instead snapshot this contract's token balance before/after the expected transfer and use the actualReceived = newBalance - oldBalance (or min(actualReceived, recoveredAmount_)) when updating state to ensure internalCash never exceeds the real on-chain balance. Update badDebt and internalCash using actualReceived (or revert if actualReceived < recoveredAmount_ if you want strictness), and reference the badDebtRecovered function, the internalCash and badDebt variables, and balanceOf(address(this)) when making the change.
🧹 Nitpick comments (2)
tests/hardhat/Tokens/reservesTest.ts (1)
116-117: Consider extracting a shared cash-setup helper to avoid drift.The same two-step setup is repeated multiple times; a small helper lowers the chance of future tests forgetting
harnessSetInternalCash.♻️ Suggested refactor
+async function setCashState(vToken, underlying, amount) { + await underlying.harnessSetBalance(vToken.address, amount); + await vToken.harnessSetInternalCash(amount); +} ... - await underlying.harnessSetBalance(vToken.address, cash); - await vToken.harnessSetInternalCash(cash); + await setCashState(vToken, underlying, cash); ... - await underlying.harnessSetBalance(vToken.address, cash); - await vToken.harnessSetInternalCash(cash); + await setCashState(vToken, underlying, cash); ... - await underlying.harnessSetBalance(vToken.address, cash); - await vToken.harnessSetInternalCash(cash); + await setCashState(vToken, underlying, cash); ... - await underlying.harnessSetBalance(vToken.address, cash); - await vToken.harnessSetInternalCash(cash); + await setCashState(vToken, underlying, cash);Also applies to: 150-151, 173-174, 196-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Tokens/reservesTest.ts` around lines 116 - 117, Multiple tests duplicate the two-step cash setup (calling underlying.harnessSetBalance(...) then vToken.harnessSetInternalCash(...)); extract a shared helper (e.g., setTokenCash or prepareTokenCash) that accepts the vToken instance, underlying token, and cash amount and performs those two calls so callers won't forget harnessSetInternalCash. Replace the duplicated pairs at the locations that call underlying.harnessSetBalance and vToken.harnessSetInternalCash (references: harnessSetBalance, harnessSetInternalCash, vToken, underlying) with calls to the new helper to keep setup consistent across tests.tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/opmainnet.ts (1)
3-11: Consider consolidating network wrapper boilerplate into a shared helper (optional).This gate-and-run pattern is repeated across many fork entrypoints; centralizing it would reduce config drift risk and simplify future network additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/opmainnet.ts` around lines 3 - 11, The file duplicates the gate-and-run pattern using FORK, FORKED_NETWORK and runStorageCheckTests; extract this boilerplate into a shared helper (e.g., a function like runForkedTest or runIfForked) that accepts the target network string, blockNumber, and vTokenArgs and performs the FORK && process.env.FORKED_NETWORK === FORKED_NETWORK check before calling runStorageCheckTests; update this file to call that helper with FORKED_NETWORK, 149078360 and the vTokenArgs so future fork entrypoints can reuse the helper and avoid config drift.
🤖 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/VToken.sol`:
- Around line 701-713: syncCash() is intended as a one-time bootstrap migration
but is callable repeatedly; prevent re-use by adding a one-time guard or
revoking ACM permission in the same transaction: add a state flag (e.g. bool
cashSyncedOnce) and require(!cashSyncedOnce) in syncCash(), set cashSyncedOnce =
true after updating internalCash and emitting CashSynced, or alternatively call
the AccessControlManager revoke method (via the same AccessControlManager
reference used for _checkAccessAllowed) from inside syncCash() to remove the
"syncCash()" permission after success; reference function syncCash(), state
internalCash, event CashSynced and the AccessControlManager access-check to
locate where to add the guard or revoke.
In `@contracts/VTokenInterfaces.sol`:
- Around line 134-137: Update the internalCash docstring to accurately list all
functions that mutate it: mention that it is updated via
_doTransferIn/_doTransferOut as well as syncCash() and badDebtRecovered(), and
that it must be initialized via syncCash() after upgrade; ensure the comment
near the internalCash declaration references the internalCash variable and the
three mutating functions (_doTransferIn, _doTransferOut, syncCash,
badDebtRecovered) so the write paths are correct.
In `@deploy/023-market-upgrade.ts`:
- Around line 13-15: The code may compute a block-based maxBorrowRateMantissa
when the deployment is actually time-based; update the deployment check so that
after calling getBlockOrTimestampBasedDeploymentInfo(...) you enforce
consistency: if isTimeBased is true then ensure the environment/flag
IS_TIME_BASED_DEPLOYMENT is set (or call getMaxBorrowRateMantissa in its
time-based mode) and otherwise throw/exit with a clear error; reference
getBlockOrTimestampBasedDeploymentInfo, isTimeBased, getMaxBorrowRateMantissa
and maxBorrowRateMantissa and add a guard that prevents continuing when
isTimeBased !== Boolean(IS_TIME_BASED_DEPLOYMENT) so the upgrade cannot proceed
with mismatched config.
In `@tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts`:
- Around line 157-170: The code currently skips adding markets to allMarkets
when underlying.decimals() reverts; instead of using `continue` in the catch
block for `decimals = await underlyingToken.decimals()` remove the `continue`,
assign a sentinel (e.g., `decimals = undefined` or `null`) to indicate unknown
precision, still call `getBalanceSlot` and `getMarketPauseStatus`, and push the
market into `allMarkets` as before; then update downstream amount-based checks
(the suites that iterate `allMarkets` later) to guard on the `decimals` field
and skip or handle markets where `decimals` is unset so those markets are
processed for upgrade/sync/ACL checks but excluded from
token-precision-dependent computations.
- Around line 73-123: The runner runDonationAttackOnAllMarkets iterates
testableMarkets without skipping empty markets, causing assertions to fail for
markets with totalSupply == 0; update runDonationAttackOnAllMarkets to skip any
market whose await market.vToken.totalSupply() equals zero (or
BigNumber.isZero), only perform the snapshot/donation/assert sequence for
non-empty markets, and track a counter of exercised markets to return that count
instead of testableMarkets.length; reference runDonationAttackOnAllMarkets,
testableMarkets, market.vToken.totalSupply(), exchangeRateStored(), and
exchangeRateCurrent() to locate and modify the logic.
In `@tests/hardhat/Fork/ExchangeRateManipulation/README.md`:
- Around line 9-11: The markdown fenced code blocks containing the exchangeRate
formula "exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) /
totalSupply" and the subsequent numbered scenario starting "1. Attacker calls
IERC20(underlying).transfer(vToken, largeAmount)" should be labeled with a
language to satisfy markdownlint MD040; update the opening backticks for both
fences from ``` to ```text so both examples are fenced as text blocks.
In
`@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts`:
- Around line 117-127: The code currently skips adding markets whose
underlying.decimals() reverts to upgradedMarkets, excluding them from all
storage/upgrade tests; instead, call getBalanceSlot(...) and always collect the
market into a general list (e.g., allUpgradedMarkets) and also push into a
separate decimalsRequired list only when decimals was successfully read, so only
the tests that truly need decimals (the suites that call decimals-dependent
flows) iterate over decimalsRequired while the
storage-preservation/syncCash/accrueInterest/donation-blocking suites iterate
over allUpgradedMarkets; update references to upgradedMarkets in the suites at
lines referenced to use the appropriate new collection names and keep symbols
like underlying.decimals, getBalanceSlot, vToken, name, address/marketAddr
unchanged.
- Around line 340-384: The tests iterate upgradedMarkets and increment verified
but never assert it, so add an explicit coverage assertion after the loop in the
"borrow decreases internalCash" test to require verified >= 3 (or fail the test)
so skipping all markets or all try/catch failures does not pass silently; do the
same change for the corresponding repay test (the one around lines 386-440) to
assert verified was incremented sufficiently. Locate the tests by the it("borrow
decreases internalCash", ...) and the repay test, and add a final
expect(verified).to.be.gte(3) (or similar) after the loop to enforce coverage.
---
Outside diff comments:
In `@contracts/VToken.sol`:
- Around line 655-663: The badDebtRecovered function trusts recoveredAmount_ and
immediately increases internalCash and reduces badDebt; instead snapshot this
contract's token balance before/after the expected transfer and use the
actualReceived = newBalance - oldBalance (or min(actualReceived,
recoveredAmount_)) when updating state to ensure internalCash never exceeds the
real on-chain balance. Update badDebt and internalCash using actualReceived (or
revert if actualReceived < recoveredAmount_ if you want strictness), and
reference the badDebtRecovered function, the internalCash and badDebt variables,
and balanceOf(address(this)) when making the change.
---
Nitpick comments:
In
`@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/opmainnet.ts`:
- Around line 3-11: The file duplicates the gate-and-run pattern using FORK,
FORKED_NETWORK and runStorageCheckTests; extract this boilerplate into a shared
helper (e.g., a function like runForkedTest or runIfForked) that accepts the
target network string, blockNumber, and vTokenArgs and performs the FORK &&
process.env.FORKED_NETWORK === FORKED_NETWORK check before calling
runStorageCheckTests; update this file to call that helper with FORKED_NETWORK,
149078360 and the vTokenArgs so future fork entrypoints can reuse the helper and
avoid config drift.
In `@tests/hardhat/Tokens/reservesTest.ts`:
- Around line 116-117: Multiple tests duplicate the two-step cash setup (calling
underlying.harnessSetBalance(...) then vToken.harnessSetInternalCash(...));
extract a shared helper (e.g., setTokenCash or prepareTokenCash) that accepts
the vToken instance, underlying token, and cash amount and performs those two
calls so callers won't forget harnessSetInternalCash. Replace the duplicated
pairs at the locations that call underlying.harnessSetBalance and
vToken.harnessSetInternalCash (references: harnessSetBalance,
harnessSetInternalCash, vToken, underlying) with calls to the new helper to keep
setup consistent across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ee87851-3cea-4171-ba9a-cf6bee302fef
📒 Files selected for processing (37)
contracts/VToken.solcontracts/VTokenInterfaces.solcontracts/test/VTokenHarness.soldeploy/023-market-upgrade.tsdeployments/basemainnet.jsondeployments/basemainnet/VTokenImpl.jsondeployments/basemainnet/solcInputs/924fda9e1aba8284e35d228bbb79aacd.jsondeployments/basemainnet_addresses.jsondeployments/opmainnet.jsondeployments/opmainnet/VTokenImpl.jsondeployments/opmainnet/solcInputs/924fda9e1aba8284e35d228bbb79aacd.jsondeployments/opmainnet_addresses.jsonhardhat.config.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/arbitrumone.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/basemainnet.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/ethereum.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/helpers.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/opbnbmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/opmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/unichainmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/zksyncmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/README.mdtests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/arbitrumone.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/basemainnet.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/ethereum.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/helpers.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/opbnbmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/opmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/unichainmainnet.tstests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/zksyncmainnet.tstests/hardhat/Tokens/borrowAndRepayTest.tstests/hardhat/Tokens/internalCashTest.tstests/hardhat/Tokens/liquidateTest.tstests/hardhat/Tokens/mintAndRedeemTest.tstests/hardhat/Tokens/reservesTest.ts
| const { isTimeBased, blocksPerYear } = getBlockOrTimestampBasedDeploymentInfo(hre.getNetworkName()); | ||
| const maxBorrowRateMantissa = getMaxBorrowRateMantissa(hre.network.name); | ||
|
|
There was a problem hiding this comment.
Guard against time-based/max-borrow-rate config mismatch.
Line 14 can produce a non-time-based maxBorrowRateMantissa when isTimeBased is true but IS_TIME_BASED_DEPLOYMENT is not set. This is a deployment-time correctness risk for live upgrades.
Proposed fix
const { isTimeBased, blocksPerYear } = getBlockOrTimestampBasedDeploymentInfo(hre.getNetworkName());
+ if (isTimeBased && process.env.IS_TIME_BASED_DEPLOYMENT !== "true") {
+ throw new Error(
+ "Time-based deployment detected, but IS_TIME_BASED_DEPLOYMENT is not true. Refusing to deploy with inconsistent maxBorrowRateMantissa.",
+ );
+ }
const maxBorrowRateMantissa = getMaxBorrowRateMantissa(hre.network.name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { isTimeBased, blocksPerYear } = getBlockOrTimestampBasedDeploymentInfo(hre.getNetworkName()); | |
| const maxBorrowRateMantissa = getMaxBorrowRateMantissa(hre.network.name); | |
| const { isTimeBased, blocksPerYear } = getBlockOrTimestampBasedDeploymentInfo(hre.getNetworkName()); | |
| if (isTimeBased && process.env.IS_TIME_BASED_DEPLOYMENT !== "true") { | |
| throw new Error( | |
| "Time-based deployment detected, but IS_TIME_BASED_DEPLOYMENT is not true. Refusing to deploy with inconsistent maxBorrowRateMantissa.", | |
| ); | |
| } | |
| const maxBorrowRateMantissa = getMaxBorrowRateMantissa(hre.network.name); | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/023-market-upgrade.ts` around lines 13 - 15, The code may compute a
block-based maxBorrowRateMantissa when the deployment is actually time-based;
update the deployment check so that after calling
getBlockOrTimestampBasedDeploymentInfo(...) you enforce consistency: if
isTimeBased is true then ensure the environment/flag IS_TIME_BASED_DEPLOYMENT is
set (or call getMaxBorrowRateMantissa in its time-based mode) and otherwise
throw/exit with a clear error; reference getBlockOrTimestampBasedDeploymentInfo,
isTimeBased, getMaxBorrowRateMantissa and maxBorrowRateMantissa and add a guard
that prevents continuing when isTimeBased !== Boolean(IS_TIME_BASED_DEPLOYMENT)
so the upgrade cannot proceed with mismatched config.
There was a problem hiding this comment.
This is a pre-existing pattern across all deploy scripts in the repo (001 through 022) — getBlockOrTimestampBasedDeploymentInfo() reads from a hardcoded lookup table while getMaxBorrowRateMantissa() reads from IS_TIME_BASED_DEPLOYMENT env var. The two sources are always set together by the deployment tooling.
There was a problem hiding this comment.
@Debugger022, understood — thanks for the context! Since both sources are always aligned by the deployment tooling and this pattern is consistent across all existing scripts (001–022), the guard I suggested would be redundant. I'll note this for future reviews.
(∩`-´)⊃━☆ﾟ.*･｡ﾟ
✏️ Learnings added
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: deploy/023-market-upgrade.ts:13-15
Timestamp: 2026-03-19T07:18:54.783Z
Learning: In `VenusProtocol/isolated-pools`, all deploy scripts follow a pattern where `getBlockOrTimestampBasedDeploymentInfo()` reads `isTimeBased` and `blocksPerYear` from a hardcoded network lookup table, while `getMaxBorrowRateMantissa()` reads from the `IS_TIME_BASED_DEPLOYMENT` environment variable. These two sources are always set together by the deployment tooling and are considered always consistent. Do not flag this as a mismatch risk or suggest adding consistency guards — it is an established, intentional pattern across all deploy scripts (001–022 and beyond).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: contracts/VToken.sol:701-713
Timestamp: 2026-03-19T07:14:16.081Z
Learning: In `contracts/VToken.sol` (Venus Protocol isolated-pools), `syncCash()` is intentionally kept re-callable (not a one-time guard). It is ACM-gated via governance timelocks, the same trust boundary that controls upgrades, pausing, and reserve management. Re-callability is by design to support future reconciliation scenarios such as airdropped tokens or future integrations that bypass `_doTransferIn`. Do not flag this as a bug or suggest adding a one-time guard.
| ``` | ||
| exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) / totalSupply | ||
| ``` |
There was a problem hiding this comment.
Add languages to the fenced examples.
These two unlabeled fences trip markdownlint MD040. Using text here is enough to keep the new README lint-clean.
✏️ Suggested edit
-```
+```text
exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) / totalSupply- +text
-
Attacker calls IERC20(underlying).transfer(vToken, largeAmount)
→ vToken.balanceOf increases, but no vTokens are minted
→ Supply cap is NOT checked (only checked during mint/mintBehalf) -
exchangeRate spikes because totalCash increased while totalSupply stayed the same
-
Attacker (or accomplice) redeems vTokens at the inflated rate
→ Receives more underlying than was legitimately deposited
→ Remaining depositors are left with a shortfall (bad debt)
Also applies to: 17-27
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/ExchangeRateManipulation/README.md` around lines 9 - 11,
The markdown fenced code blocks containing the exchangeRate formula
"exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) /
totalSupply" and the subsequent numbered scenario starting "1. Attacker calls
IERC20(underlying).transfer(vToken, largeAmount)" should be labeled with a
language to satisfy markdownlint MD040; update the opening backticks for both
fences from ``` to ```text so both examples are fenced as text blocks.
There was a problem hiding this comment.
Not a functional issue and not relevant to the PR's scope.
There was a problem hiding this comment.
@Debugger022, understood — it's a purely cosmetic lint suggestion with no functional impact. Happy to leave it as-is.
🧠 Learnings used
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts:73-123
Timestamp: 2026-03-19T07:21:51.859Z
Learning: In `tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts` (Venus Protocol isolated-pools), the `runDonationAttackOnAllMarkets` helper intentionally does NOT guard against `totalSupply == 0` markets. On pinned mainnet fork blocks, every Core pool market that passes the existing filters (not paused + has a known balance slot) is guaranteed to have live deposits, making a zero-supply check unnecessary and unrealistic for these fork tests.
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts:157-170
Timestamp: 2026-03-19T07:23:15.598Z
Learning: In `tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts` (Venus Protocol isolated-pools), the `continue` in the `catch` block for `underlyingToken.decimals()` is intentional and by design. Markets with non-standard underlying proxies that revert on `decimals()` are deliberately excluded from `allMarkets` in this suite because every downstream computation (donations, mints, borrows, funding via `parseUnits`) requires known precision. Upgrade/syncCash/ACL coverage for these exotic markets is provided separately by the `vTokenStorageChecks/testRunner.ts` suite, which iterates `allMarketAddresses` directly from the comptroller and runs `syncCash` on every market before filtering out decimals-broken markets for normal-ops tests.
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: contracts/VToken.sol:701-713
Timestamp: 2026-03-19T07:14:16.081Z
Learning: In `contracts/VToken.sol` (Venus Protocol isolated-pools), `syncCash()` is intentionally kept re-callable (not a one-time guard). It is ACM-gated via governance timelocks, the same trust boundary that controls upgrades, pausing, and reserve management. Re-callability is by design to support future reconciliation scenarios such as airdropped tokens or future integrations that bypass `_doTransferIn`. Do not flag this as a bug or suggest adding a one-time guard.
| try { | ||
| decimals = await underlyingToken.decimals(); | ||
| } catch { | ||
| // Some underlying tokens (e.g. vweETH_Core on Unichain) use non-standard proxies | ||
| // that revert on decimals(). Skip these markets from normal ops testing. | ||
| console.log(` ⏭ ${name}: skipped from normal ops (underlying.decimals() reverted)`); | ||
| continue; | ||
| } | ||
|
|
||
| const balanceSlot = await getBalanceSlot(underlying, config.network); | ||
| upgradedMarkets.push({ name, address: marketAddr, underlying, decimals, vToken, balanceSlot }); |
There was a problem hiding this comment.
Keep non-standard underlyings in the upgrade/storage coverage.
The comment says these markets should only be skipped from normal-ops testing, but Line 123 continues before Line 127, so they never enter upgradedMarkets. Because upgradedMarkets is then reused by the suites at Lines 135, 151, 179, and 190, any market whose underlying.decimals() reverts is excluded from storage preservation, syncCash, accrueInterest, and donation-blocking checks entirely. Split this into separate collections so only the flows that truly need decimals are skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts`
around lines 117 - 127, The code currently skips adding markets whose
underlying.decimals() reverts to upgradedMarkets, excluding them from all
storage/upgrade tests; instead, call getBalanceSlot(...) and always collect the
market into a general list (e.g., allUpgradedMarkets) and also push into a
separate decimalsRequired list only when decimals was successfully read, so only
the tests that truly need decimals (the suites that call decimals-dependent
flows) iterate over decimalsRequired while the
storage-preservation/syncCash/accrueInterest/donation-blocking suites iterate
over allUpgradedMarkets; update references to upgradedMarkets in the suites at
lines referenced to use the appropriate new collection names and keep symbols
like underlying.decimals, getBalanceSlot, vToken, name, address/marketAddr
unchanged.
There was a problem hiding this comment.
syncCash() already runs on all markets via allMarketAddresses before the decimals filter (lines 106-110). Splitting upgradedMarkets into two lists adds maintenance overhead for one exotic token on one chain. No change.
There was a problem hiding this comment.
@Debugger022, you're right — syncCash() is called on every entry in allMarketAddresses (lines 106–110) before the decimals() guard is ever reached, so the sync coverage for exotic markets is already complete. The continue only gates them out of the downstream normal-ops flows that require known precision. Acknowledged, no change needed.
🧠 Learnings used
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts:157-170
Timestamp: 2026-03-19T07:23:15.598Z
Learning: In `tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts` (Venus Protocol isolated-pools), the `continue` in the `catch` block for `underlyingToken.decimals()` is intentional and by design. Markets with non-standard underlying proxies that revert on `decimals()` are deliberately excluded from `allMarkets` in this suite because every downstream computation (donations, mints, borrows, funding via `parseUnits`) requires known precision. Upgrade/syncCash/ACL coverage for these exotic markets is provided separately by the `vTokenStorageChecks/testRunner.ts` suite, which iterates `allMarketAddresses` directly from the comptroller and runs `syncCash` on every market before filtering out decimals-broken markets for normal-ops tests.
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts:73-123
Timestamp: 2026-03-19T07:21:51.859Z
Learning: In `tests/hardhat/Fork/ExchangeRateManipulation/DonationAttack/testRunner.ts` (Venus Protocol isolated-pools), the `runDonationAttackOnAllMarkets` helper intentionally does NOT guard against `totalSupply == 0` markets. On pinned mainnet fork blocks, every Core pool market that passes the existing filters (not paused + has a known balance slot) is guaranteed to have live deposits, making a zero-supply check unnecessary and unrealistic for these fork tests.
Learnt from: Debugger022
Repo: VenusProtocol/isolated-pools PR: 551
File: contracts/VToken.sol:701-713
Timestamp: 2026-03-19T07:14:16.081Z
Learning: In `contracts/VToken.sol` (Venus Protocol isolated-pools), `syncCash()` is intentionally kept re-callable (not a one-time guard). It is ACM-gated via governance timelocks, the same trust boundary that controls upgrades, pausing, and reserve management. Re-callability is by design to support future reconciliation scenarios such as airdropped tokens or future integrations that bypass `_doTransferIn`. Do not flag this as a bug or suggest adding a one-time guard.
- List all four mutating functions in internalCash NatSpec - Fix README fork test commands to use correct env vars (FORK=true, FORKED_NETWORK, HARDHAT_FORK_NETWORK) so tests actually execute - Add missing verified > 0 assertions to borrow/repay fork tests to prevent silent zero-coverage passes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts (1)
117-127:⚠️ Potential issue | 🟠 Major
decimals()failures still remove these markets from the non-precision suites.This
continuehappens aftersyncCash()but beforeupgradedMarkets.push(...), so the loops at Line 135, Line 151, Line 179, and Line 190 never assert storage preservation,syncCashalignment,accrueInterest, or donation immunity for those markets. Only the mint/borrow/repay flows need token precision, so this should keep anallUpgradedMarketslist for the generic assertions and use a separate decimals-ready list for normal ops.Based on learnings, markets whose
underlying.decimals()revert are only meant to be excluded from precision-dependent flows; upgrade/syncCash/ACL coverage should still come from this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts` around lines 117 - 127, The current early `continue` after catching `underlying.decimals()` prevents markets from being included in generic assertions (storage preservation, syncCash alignment, accrueInterest, donation immunity); change the logic to always push the market into a general list (e.g., create/append to allUpgradedMarkets) so upgrade/syncCash/ACL tests still run, and only push into the precision-dependent list (e.g., decimalsReady or upgradedMarketsWithDecimals) when `decimals()` succeeds; update downstream loops (the ones at the locations that currently iterate upgradedMarkets for normal ops) to use the precision-ready list and leave the generic assertion loops using allUpgradedMarkets so markets that revert on decimals() are excluded only from precision flows but still get coverage for syncCash(), accrueInterest(), and storage checks.
🧹 Nitpick comments (2)
tests/hardhat/Fork/ExchangeRateManipulation/README.md (1)
48-54: DocumentsyncCash()as re-callable, not one-time.These lines describe
syncCash()as a one-time migration call, but the implementation intentionally leaves it callable again for future reconciliations. As written, the README understates the supported operational model.✏️ Suggested edit
-| `syncCash()` | One-time ACM-gated call: `internalCash = balanceOf(address(this))` | +| `syncCash()` | ACM-gated reconciliation call: `internalCash = balanceOf(address(this))` | -`syncCash()` is a migration function — called once after upgrading each vToken to bootstrap `internalCash` from the existing balance. It's gated by AccessControlManager so only authorized callers can invoke it. +`syncCash()` is used during the upgrade migration to bootstrap `internalCash` from the existing balance. It intentionally remains available for future reconciliations, and AccessControlManager restricts it to authorized callers.Based on learnings,
syncCash()is intentionally kept re-callable to support future reconciliation scenarios such as airdropped tokens or integrations that bypass_doTransferIn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/ExchangeRateManipulation/README.md` around lines 48 - 54, Update the README to state that syncCash() is re-callable (not strictly one-time): explain that syncCash() can be called multiple times to resynchronize internalCash with balanceOf(address(this)) for future reconciliations (e.g., airdropped tokens or transfers bypassing _doTransferIn), that it remains gated by AccessControlManager, and that vToken upgrades should still call it initially to bootstrap internalCash but may call it again as needed.tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts (1)
57-64: Fail fast when the fork RPC URL is missing.If
ARCHIVE_NODE_${config.network}is unset, this fails later insidehardhat_resetwith a generic forking error. A direct precondition check here makes fork misconfiguration much easier to diagnose.✏️ Suggested edit
before(async () => { // 1. Fork at pinned block + const archiveNodeUrl = process.env[`ARCHIVE_NODE_${config.network}`]; + if (!archiveNodeUrl) { + throw new Error(`Missing ARCHIVE_NODE_${config.network}`); + } + await ethers.provider.send("hardhat_reset", [ { forking: { - jsonRpcUrl: process.env[`ARCHIVE_NODE_${config.network}`], + jsonRpcUrl: archiveNodeUrl, blockNumber: config.blockNumber, }, }, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts` around lines 57 - 64, Before calling ethers.provider.send("hardhat_reset", ...) ensure the fork RPC URL exists by reading process.env[`ARCHIVE_NODE_${config.network}`] and throwing a clear, early error if it's missing; update the testRunner.ts code around the hardhat_reset call (refer to the config.network usage and the hardhat_reset invocation) to validate the environment variable and fail fast with a descriptive message like "missing ARCHIVE_NODE_<network> - set ARCHIVE_NODE_<network> to your archive node URL" so misconfiguration is diagnosed immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts`:
- Around line 117-127: The current early `continue` after catching
`underlying.decimals()` prevents markets from being included in generic
assertions (storage preservation, syncCash alignment, accrueInterest, donation
immunity); change the logic to always push the market into a general list (e.g.,
create/append to allUpgradedMarkets) so upgrade/syncCash/ACL tests still run,
and only push into the precision-dependent list (e.g., decimalsReady or
upgradedMarketsWithDecimals) when `decimals()` succeeds; update downstream loops
(the ones at the locations that currently iterate upgradedMarkets for normal
ops) to use the precision-ready list and leave the generic assertion loops using
allUpgradedMarkets so markets that revert on decimals() are excluded only from
precision flows but still get coverage for syncCash(), accrueInterest(), and
storage checks.
---
Nitpick comments:
In `@tests/hardhat/Fork/ExchangeRateManipulation/README.md`:
- Around line 48-54: Update the README to state that syncCash() is re-callable
(not strictly one-time): explain that syncCash() can be called multiple times to
resynchronize internalCash with balanceOf(address(this)) for future
reconciliations (e.g., airdropped tokens or transfers bypassing _doTransferIn),
that it remains gated by AccessControlManager, and that vToken upgrades should
still call it initially to bootstrap internalCash but may call it again as
needed.
In
`@tests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts`:
- Around line 57-64: Before calling ethers.provider.send("hardhat_reset", ...)
ensure the fork RPC URL exists by reading
process.env[`ARCHIVE_NODE_${config.network}`] and throwing a clear, early error
if it's missing; update the testRunner.ts code around the hardhat_reset call
(refer to the config.network usage and the hardhat_reset invocation) to validate
the environment variable and fail fast with a descriptive message like "missing
ARCHIVE_NODE_<network> - set ARCHIVE_NODE_<network> to your archive node URL" so
misconfiguration is diagnosed immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2daada4-cfef-438e-a3c1-9f6ee9e4ab11
📒 Files selected for processing (3)
contracts/VTokenInterfaces.soltests/hardhat/Fork/ExchangeRateManipulation/README.mdtests/hardhat/Fork/ExchangeRateManipulation/vTokenStorageChecks/testRunner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/VTokenInterfaces.sol
Deploy VToken with internalCash fix to ethereum mainnet at 0x33bE30B31f07c8a2bfb705FBcE55E983c47ba864
|
2 participants
Replace
balanceOf(address(this))with an internally trackedinternalCashvariable in_getCashPrior()to make exchange rate, borrow, redeem, and interest rate calculations immune to direct token transfers (donation attacks).Contract changes:
internalCashstorage variable to VTokenStorage (gap 48→47)_doTransferIn: incrementinternalCashby actual amount received_doTransferOut: decrementinternalCashbefore transfer_getCashPrior: returninternalCashinstead ofbalanceOfsyncCash()for one-time initialization after upgradeCashSyncedevent andsyncCash()to VTokenInterfaceTest changes:
harnessSetInternalCash()to VTokenHarnessinternalCashin existing borrow, redeem, liquidate, reserves testsSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests & Docs