[VPD-808] Add internal cash to prevent donation attack#664
[VPD-808] Add internal cash to prevent donation attack#664fred-venus merged 19 commits intodevelopfrom
Conversation
…messages and additional markets - Add descriptive error messages to all assertions for clearer failure diagnostics - Add missing markets (vTUSDOLD, vTRXOLD, vBCH, vLUNA, vUNI, vUST, vWBETH) - Use expect().to.not.be.reverted for accrueInterest assertions - Log fork block number in before() hook for reproducibility
Rewrite sweepToken using inline assembly to fit within EIP-170 24KB contract size limit. syncCash remains in high-level Solidity. sweepToken uses ACM for access control via raw staticcall.
The token parameter was always required to equal underlying, making it unnecessary. Using underlying directly reduces contract size and removes a superfluous check. Updated TokenSwept event accordingly.
The sweepToken assembly used hardcoded selector 0x7e131436 which does not match isAllowedToCall(address,string). The correct selector is 0x18c5e8ab.
The transfer calldata construction writes to memory offset 0x24-0x43 via mstore(0x24, excess), which partially overwrites the Solidity free memory pointer at 0x40. This causes the subsequent emit to use a corrupted memory pointer and revert. Restore it from ptr at the end.
Add unit tests for sweepToken() and syncCash() covering access control, event emission, excess recovery, idempotency, and ACM denial. Uses delegator proxy pattern to stay under EVM contract size limit. Also add vXAUM (0x92e6Ea74a1A3047DabF4186405a21c7D63a0612A) to the MARKET_PROXIES arrays in both fork test files.
Use accurate listed-market data from Comptroller (43 markets), fix exchange rate drift by using accrueInterest + exchangeRateStored, and restructure test output into clean logical groupings.
Consolidates two separate admin functions into a single sweepTokenAndSync(transferAmount) that optionally transfers excess tokens then syncs internalCash. Replaces hand-rolled assembly with Solidity using safeTransfer, simplifying auditing and maintenance.
Merge internalCashUpgrade tests into donationAttack fork suite that demonstrates the attack succeeding before upgrade and failing after. Update unit tests for the renamed sweepTokenAndSync function and add vToken storage layout checks.
|
@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 |
getCashPrior, doTransferIn, and doTransferOut now use internalCash instead of balanceOf. Update their natspec to document this behavior and remove stale comments about excluding msg.value.
New implementation with internalCash tracking and sweepTokenAndSync, deployed and verified on BscScan.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this changes the core exchange rate calculation (getCashPrior) and storage layout for all mainnet markets — a human should verify the upgrade path and migration strategy before merging.
Extended reasoning...
Overview
This PR introduces an internalCash state variable in VBep20 that replaces direct balanceOf() calls in getCashPrior(), mitigating donation/direct-transfer attacks that can inflate exchange rates. It also adds a sweepTokenAndSync(transferAmount) admin function for post-upgrade migration (pass 0 to sync) and excess token recovery. The __gap is correctly reduced from 46 to 45. Test harnesses, unit tests, and comprehensive fork tests covering 40+ BSC mainnet core pool markets are included.
Security risks
The internalCash pattern is well-established in DeFi (Compound V3, etc.) and the implementation looks correct: doTransferIn increments, doTransferOut decrements before transfer, sweepTokenAndSync is admin-only with require(msg.sender == admin). Flash loan interactions are sound — _getCashPriorWithFlashLoan() adds flashLoanAmount back to getCashPrior(), maintaining correct exchange rate during active loans. No injection, auth bypass, or data exposure risks identified.
Level of scrutiny
This PR touches the most critical code path in a lending protocol — how cash is tracked and exchange rates are computed. A subtle bug here could cause incorrect exchange rates, stuck funds, or exploitable conditions across all 40+ production markets. The storage layout change and required governance-driven migration (_setImplementation + sweepTokenAndSync(0)) add deployment risk. This demands thorough human review of both the code and the deployment/migration plan.
Other factors
The test coverage is impressive: fork tests validate before/after upgrade behavior on all real mainnet markets, storage layout checks verify no slot collisions, and unit tests cover sweep/sync edge cases. Reviewer fred-venus has been actively engaged and provided feedback that was addressed (merging syncCash + sweepToken). However, the overall scope — modifying core VToken logic, storage layout, and requiring coordinated governance upgrades — makes this unsuitable for automated approval.
|
✅ Actions performedReview triggered.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds internal cash accounting to VBep20 tokens (new Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant VToken as VToken/Proxy
participant ERC20 as UnderlyingToken
Admin->>VToken: sweepTokenAndSync(transferAmount)
activate VToken
alt transferAmount > 0
VToken->>ERC20: transfer(Admin, transferAmount)
ERC20-->>VToken: success
VToken->>VToken: emit TokenSwept(Admin, transferAmount)
end
VToken->>ERC20: balanceOf(address(this))
ERC20-->>VToken: actualBalance
VToken->>VToken: old = internalCash
VToken->>VToken: internalCash = actualBalance
VToken->>VToken: emit CashSynced(old, actualBalance)
deactivate VToken
sequenceDiagram
participant Donor
participant ERC20 as UnderlyingToken
participant VToken as VToken (post-upgrade)
Donor->>ERC20: transfer(vTokenAddress, donatedAmount)
ERC20-->>Donor: success
Donor->>VToken: getCash()
VToken-->>Donor: return internalCash
note over ERC20,VToken: ERC20.balanceOf(vToken) = internalCash + donatedAmount\nVToken.internalCash unchanged — donation ignored by accounting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 review this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (9)
contracts/test/VBep20MockDelegate.sol (1)
3-7: Remove duplicate IERC20 import.
IERC20is imported twice (lines 3 and 7). While this doesn't cause a compilation error, it's unnecessary clutter.🧹 Proposed fix
pragma solidity 0.8.25; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { ComptrollerInterface } from "../Comptroller/ComptrollerInterface.sol"; -import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { VToken } from "../Tokens/VTokens/VToken.sol";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/VBep20MockDelegate.sol` around lines 3 - 7, The imports in VBep20MockDelegate.sol include IERC20 twice; remove the redundant import of IERC20 so it's imported only once (keep the single IERC20 import alongside SafeERC20 and ComptrollerInterface), updating the import block that references IERC20 and SafeERC20 to eliminate the duplicate line.contracts/Tokens/VTokens/VBep20.sol (1)
259-270: Consider integrating sweepTokenAndSync with Access Control Manager for consistency.The
sweepTokenAndSyncfunction uses a directrequire(msg.sender == admin)check. However, other admin functions in the VToken codebase use the Access Control Manager pattern viaensureAllowed()(e.g.,_setReserveFactor,_reduceReserves,setFlashLoanEnabled). Aligning this function with the ACM pattern would provide consistency and enable granular permission control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/Tokens/VTokens/VBep20.sol` around lines 259 - 270, The direct admin check in sweepTokenAndSync should be replaced with the Access Control Manager call to match other admin flows: remove require(msg.sender == admin) and invoke ensureAllowed(...) at the start of sweepTokenAndSync (using the same role/permission identifier used by other admin functions, e.g., the ADMIN_ROLE or the same permission string used by _setReserveFactor/_reduceReserves), then proceed with the existing transfer and cash-sync logic so behavior and events (TokenSwept, CashSynced) remain unchanged.tests/hardhat/Fork/vTokenStorageChecks.ts (3)
69-99: Code duplication:findBalanceSlotandsetTokenBalanceare duplicated indonationAttack.ts.Both
findBalanceSlotandsetTokenBalancehelper functions are identical to those intests/hardhat/Fork/donationAttack.ts. Consider extracting them totests/hardhat/Fork/utils.tsfor reuse and maintainability.Suggested refactor
Add to
tests/hardhat/Fork/utils.ts:export async function findBalanceSlot(tokenAddress: string): Promise<number | null> { // ... implementation } export async function setTokenBalance(tokenAddress: string, account: string, amount: BigNumber, slot: number) { // ... implementation }Then import in both test files:
-import { FORK_MAINNET, forking, initMainnetUser } from "./utils"; +import { FORK_MAINNET, forking, initMainnetUser, findBalanceSlot, setTokenBalance } from "./utils";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/vTokenStorageChecks.ts` around lines 69 - 99, The findBalanceSlot and setTokenBalance helpers are duplicated; extract both functions (findBalanceSlot and setTokenBalance) into a shared module (e.g., tests/hardhat/Fork/utils.ts) as exported functions, update both tests (vTokenStorageChecks.ts and donationAttack.ts) to import these functions instead of defining them locally, and ensure the new module exports the same signatures (Promise<number|null> for findBalanceSlot and async setTokenBalance(tokenAddress: string, account: string, amount: BigNumber, slot: number)) so existing calls continue to work.
182-186: Consider logging full error for debugging purposes.The error message is truncated to 80 characters which may hide important diagnostic information. Consider logging the full error or at minimum the error type.
Suggested improvement
} catch (e: any) { - console.log(` Failed to upgrade ${market.name}: ${e.message?.slice(0, 80)}`); + console.log(` Failed to upgrade ${market.name}: ${e.message || e}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/vTokenStorageChecks.ts` around lines 182 - 186, The catch block that currently logs a truncated error message (catch (e: any) { console.log(` Failed to upgrade ${market.name}: ${e.message?.slice(0, 80)}`); }) should be changed to log the full error and type for debugging: replace the truncated e.message with either e.stack (preferred) or a combination of e.name and e.message, and use console.error instead of console.log to indicate failure; update the log to something like: console.error(`Failed to upgrade ${market.name}:`, e) or console.error(`Failed to upgrade ${market.name}: ${e.name} - ${e.message}`, e.stack) so full diagnostic data is preserved.
10-10: Unused import:IAccessControlManagerV5__factory.This import is not used anywhere in the file and can be removed.
Remove unused import
import { ERC20__factory, - IAccessControlManagerV5__factory, VBep20Delegate__factory, VBep20Delegator__factory, } from "../../../typechain";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/vTokenStorageChecks.ts` at line 10, Remove the unused import IAccessControlManagerV5__factory from the imports list in the file (it is declared but never referenced); locate the import statement that includes IAccessControlManagerV5__factory and delete that symbol (or the entire import line if it contains only that factory) so there are no unused imports remaining.tests/hardhat/Fork/donationAttack.ts (2)
228-232: Unusual assertion pattern:.to.be.rejectedWith("").Using
.to.be.rejectedWith("")with an empty string is unusual and may not work as intended. Consider using.to.be.revertedfor consistency with other tests in this PR, or specify the expected error message.Suggested fix
it("reverts for non-admin callers", async () => { const [, randomUser] = await ethers.getSigners(); const vToken = await ethers.getContractAt("VBep20Delegate", upgradedMarkets[0].proxy, randomUser); - await expect(vToken.sweepTokenAndSync(0)).to.be.rejectedWith(""); + await expect(vToken.sweepTokenAndSync(0)).to.be.reverted; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/donationAttack.ts` around lines 228 - 232, The test uses an odd assertion `.to.be.rejectedWith("")` which is unreliable; update the assertion for the call to VBep20Delegate.sweepTokenAndSync invoked on vToken (created with ethers.getContractAt and randomUser) to use the proper Hardhat/Chai matcher — either `.to.be.reverted()` for a generic revert or `.to.be.revertedWith("<expected revert message>")` if a specific revert reason is known — so replace the `.to.be.rejectedWith("")` on the expect(vToken.sweepTokenAndSync(0)) line accordingly.
321-353: Earlyreturnexits after first successful market.The
returnstatement at line 351 causes the test to exit after the first market is successfully verified. This is acceptable for a "happy path" sanity check, but be aware the test title suggests it tests all markets while it only tests one. Consider clarifying the test name if this is intentional.Optional: Clarify test name
- it("mint increases internalCash", async () => { + it("mint increases internalCash (first compatible market)", async () => {Same applies to the "redeem decreases internalCash" test at line 355.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/donationAttack.ts` around lines 321 - 353, The test "mint increases internalCash" currently returns after the first successful market due to the stray return inside the loop, so it only verifies one market; either remove that return to let the for-loop continue validating all entries in upgradedMarkets (keeping the try/catch so supply-cap or paused markets are skipped) or, if only a single-success check was intended, rename the test/title to reflect that behavior; apply the same change to the analogous "redeem decreases internalCash" test so both tests' behavior and names match intent, referencing the test block containing upgradedMarkets, vToken.mint, and the internalCashBefore/internalCashAfter assertions.tests/hardhat/VToken/sweepTokenAndSyncCash.ts (2)
13-14: Consider adding type annotations for signers.Adding explicit types improves code readability and IDE support.
Suggested type annotations
+import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; + describe("VToken", function () { - let root, nonAdmin; + let root: SignerWithAddress; + let nonAdmin: SignerWithAddress; let vToken: Contract;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/VToken/sweepTokenAndSyncCash.ts` around lines 13 - 14, The test declares signer variables root and nonAdmin without types; add explicit signer types (e.g., SignerWithAddress or ethers.Signer) to those variables to improve readability and IDE support, and add the corresponding type import (for example import type { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers") so the declarations like root and nonAdmin and any other signer variables have correct type annotations while leaving vToken typed as Contract.
76-78: Consider asserting specific revert reasons for better test precision.Using generic
.to.be.revertedmay pass even if the contract reverts for an unexpected reason. Asserting the specific error message ensures the correct access control check is triggered.Example improvement
it("reverts when called by non-admin", async () => { - await expect(vToken.connect(nonAdmin).sweepTokenAndSync(0)).to.be.reverted; + await expect(vToken.connect(nonAdmin).sweepTokenAndSync(0)).to.be.revertedWith( + "only admin can call this function" + ); });Apply similar changes to lines 127 and 132.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/VToken/sweepTokenAndSyncCash.ts` around lines 76 - 78, Replace the generic `.to.be.reverted` assertions in the vToken.sweepTokenAndSync tests with precise revert checks: find the actual admin-access revert string used by the contract (e.g., the Ownable or custom admin error) and use `await expect(vToken.connect(nonAdmin).sweepTokenAndSync(0)).to.be.revertedWith("<actual revert message>")`; apply the same change to the other two assertions mentioned (lines around 127 and 132) so the tests assert the specific revert reason for `sweepTokenAndSync`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/test/VBep20MockDelegate.sol`:
- Around line 3-7: The imports in VBep20MockDelegate.sol include IERC20 twice;
remove the redundant import of IERC20 so it's imported only once (keep the
single IERC20 import alongside SafeERC20 and ComptrollerInterface), updating the
import block that references IERC20 and SafeERC20 to eliminate the duplicate
line.
In `@contracts/Tokens/VTokens/VBep20.sol`:
- Around line 259-270: The direct admin check in sweepTokenAndSync should be
replaced with the Access Control Manager call to match other admin flows: remove
require(msg.sender == admin) and invoke ensureAllowed(...) at the start of
sweepTokenAndSync (using the same role/permission identifier used by other admin
functions, e.g., the ADMIN_ROLE or the same permission string used by
_setReserveFactor/_reduceReserves), then proceed with the existing transfer and
cash-sync logic so behavior and events (TokenSwept, CashSynced) remain
unchanged.
In `@tests/hardhat/Fork/donationAttack.ts`:
- Around line 228-232: The test uses an odd assertion `.to.be.rejectedWith("")`
which is unreliable; update the assertion for the call to
VBep20Delegate.sweepTokenAndSync invoked on vToken (created with
ethers.getContractAt and randomUser) to use the proper Hardhat/Chai matcher —
either `.to.be.reverted()` for a generic revert or
`.to.be.revertedWith("<expected revert message>")` if a specific revert reason
is known — so replace the `.to.be.rejectedWith("")` on the
expect(vToken.sweepTokenAndSync(0)) line accordingly.
- Around line 321-353: The test "mint increases internalCash" currently returns
after the first successful market due to the stray return inside the loop, so it
only verifies one market; either remove that return to let the for-loop continue
validating all entries in upgradedMarkets (keeping the try/catch so supply-cap
or paused markets are skipped) or, if only a single-success check was intended,
rename the test/title to reflect that behavior; apply the same change to the
analogous "redeem decreases internalCash" test so both tests' behavior and names
match intent, referencing the test block containing upgradedMarkets,
vToken.mint, and the internalCashBefore/internalCashAfter assertions.
In `@tests/hardhat/Fork/vTokenStorageChecks.ts`:
- Around line 69-99: The findBalanceSlot and setTokenBalance helpers are
duplicated; extract both functions (findBalanceSlot and setTokenBalance) into a
shared module (e.g., tests/hardhat/Fork/utils.ts) as exported functions, update
both tests (vTokenStorageChecks.ts and donationAttack.ts) to import these
functions instead of defining them locally, and ensure the new module exports
the same signatures (Promise<number|null> for findBalanceSlot and async
setTokenBalance(tokenAddress: string, account: string, amount: BigNumber, slot:
number)) so existing calls continue to work.
- Around line 182-186: The catch block that currently logs a truncated error
message (catch (e: any) { console.log(` Failed to upgrade ${market.name}:
${e.message?.slice(0, 80)}`); }) should be changed to log the full error and
type for debugging: replace the truncated e.message with either e.stack
(preferred) or a combination of e.name and e.message, and use console.error
instead of console.log to indicate failure; update the log to something like:
console.error(`Failed to upgrade ${market.name}:`, e) or console.error(`Failed
to upgrade ${market.name}: ${e.name} - ${e.message}`, e.stack) so full
diagnostic data is preserved.
- Line 10: Remove the unused import IAccessControlManagerV5__factory from the
imports list in the file (it is declared but never referenced); locate the
import statement that includes IAccessControlManagerV5__factory and delete that
symbol (or the entire import line if it contains only that factory) so there are
no unused imports remaining.
In `@tests/hardhat/VToken/sweepTokenAndSyncCash.ts`:
- Around line 13-14: The test declares signer variables root and nonAdmin
without types; add explicit signer types (e.g., SignerWithAddress or
ethers.Signer) to those variables to improve readability and IDE support, and
add the corresponding type import (for example import type { SignerWithAddress }
from "@nomiclabs/hardhat-ethers/signers") so the declarations like root and
nonAdmin and any other signer variables have correct type annotations while
leaving vToken typed as Contract.
- Around line 76-78: Replace the generic `.to.be.reverted` assertions in the
vToken.sweepTokenAndSync tests with precise revert checks: find the actual
admin-access revert string used by the contract (e.g., the Ownable or custom
admin error) and use `await
expect(vToken.connect(nonAdmin).sweepTokenAndSync(0)).to.be.revertedWith("<actual
revert message>")`; apply the same change to the other two assertions mentioned
(lines around 127 and 132) so the tests assert the specific revert reason for
`sweepTokenAndSync`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b25c26a5-7dc6-4225-b2b7-4b597a2fe594
📒 Files selected for processing (15)
contracts/Tokens/VTokens/VBep20.solcontracts/Tokens/VTokens/VTokenInterfaces.solcontracts/test/EvilXToken.solcontracts/test/VBep20Harness.solcontracts/test/VBep20MockDelegate.soldeployments/bscmainnet.jsondeployments/bscmainnet/VBep20Delegate.jsondeployments/bscmainnet/solcInputs/86bcc0e5783c73f34a3d5488fc875748.jsondeployments/bscmainnet_addresses.jsontests/hardhat/Comptroller/Diamond/flashLoan.tstests/hardhat/EvilXToken.tstests/hardhat/Fork/donationAttack.tstests/hardhat/Fork/vTokenStorageChecks.tstests/hardhat/VToken/reservesTest.tstests/hardhat/VToken/sweepTokenAndSyncCash.ts
Debugger022
changed the title
Remove redundant IERC20 import in VBep20MockDelegate and unused factory imports in vTokenStorageChecks fork test.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/hardhat/Fork/vTokenStorageChecks.ts (1)
67-94: Cache discovered balance slots to reduce fork-test cost/flakiness.
findBalanceSlotis re-run many times for the same underlying token addresses. Caching per token will materially reduce RPC/storage writes and runtime.Refactor sketch
+const balanceSlotCache = new Map<string, number | null>(); + +async function getBalanceSlotCached(tokenAddress: string): Promise<number | null> { + if (balanceSlotCache.has(tokenAddress)) return balanceSlotCache.get(tokenAddress)!; + const slot = await findBalanceSlot(tokenAddress); + balanceSlotCache.set(tokenAddress, slot); + return slot; +}Then replace each
findBalanceSlot(market.underlying)call withgetBalanceSlotCached(market.underlying).Also applies to: 272-273, 326-327, 387-388, 445-446, 506-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/vTokenStorageChecks.ts` around lines 67 - 94, findBalanceSlot is being called repeatedly for the same token addresses which causes extra RPC/storage writes and flakiness; add a simple in-memory cache (e.g., a Map<string, number | null>) and implement a wrapper getBalanceSlotCached(tokenAddress: string) that returns a cached slot if present or calls findBalanceSlot, stores the result, and returns it; update all call sites (the existing findBalanceSlot(...) invocations noted in the review) to call getBalanceSlotCached(...) instead so repeated lookups reuse the cached slot and avoid redundant setStorageAt/getStorageAt operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hardhat/Fork/vTokenStorageChecks.ts`:
- Around line 204-210: Add assertions for the captured fields that are missing
in the storage-collision checks: assert that after.interestRateModel equals
before.interestRateModel, after.borrowIndex equals before.borrowIndex, and
after.totalReserves equals before.totalReserves using the same
expect(...).to.equal(...) pattern and `${market.name}: <field>` message; do this
for both places where the snapshot comparisons occur (the blocks that already
check reserveFactorMantissa/totalBorrows/totalSupply/underlying) so the test
fully verifies interestRateModel, borrowIndex, and totalReserves remain
unchanged across the upgrade.
- Around line 237-246: The tests assume upgradedMarkets[0] exists and will throw
a TypeError if the setup upgraded none; add a guard at the start of each
affected test (e.g., the "reverts for non-admin callers" and "can be called
again by admin (re-syncs)" tests) that asserts upgradedMarkets.length > 0 (for
example using expect(upgradedMarkets.length).to.be.gt(0)) or calls this.skip()
when empty, then proceed to use upgradedMarkets[0]; reference the
upgradedMarkets array and the test blocks to locate where to add the check.
- Around line 156-182: The current loop over MARKET_PROXIES swallows upgrade
errors (occurring during snapshotViaProxy,
VBep20Delegator__factory.connect/_setImplementation, or
vToken.sweepTokenAndSync) by logging and continuing, which can mask regressions;
change the logic to collect failures (e.g., push {name, error} into a failures
array) or immediately rethrow instead of silently logging, and after the loop
assert that failures.length === 0 (or throw a consolidated error listing failed
market names and messages) so the setup fails unless failures are explicitly
allowlisted; reference MARKET_PROXIES, snapshotViaProxy,
VBep20Delegator__factory.connect, _setImplementation, vToken.sweepTokenAndSync,
and upgradedMarkets when making the change.
---
Nitpick comments:
In `@tests/hardhat/Fork/vTokenStorageChecks.ts`:
- Around line 67-94: findBalanceSlot is being called repeatedly for the same
token addresses which causes extra RPC/storage writes and flakiness; add a
simple in-memory cache (e.g., a Map<string, number | null>) and implement a
wrapper getBalanceSlotCached(tokenAddress: string) that returns a cached slot if
present or calls findBalanceSlot, stores the result, and returns it; update all
call sites (the existing findBalanceSlot(...) invocations noted in the review)
to call getBalanceSlotCached(...) instead so repeated lookups reuse the cached
slot and avoid redundant setStorageAt/getStorageAt operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d79a530b-b671-47a9-a4f2-b41ea59a3869
📒 Files selected for processing (2)
contracts/test/VBep20MockDelegate.soltests/hardhat/Fork/vTokenStorageChecks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/test/VBep20MockDelegate.sol
Move the upgradedMarkets length guard into the before() hook so it halts the entire suite on failure. Add interestRateModel and borrowIndex assertions that aren't covered by the exchange rate check.
|
3 participants
Summary
internalCashstate variable in VBep20 to track token balances internally, replacing directbalanceOfcalls ingetCashPrior()— this mitigates donation/direct-transfer attacks that inflate exchange rates.sweepTokenAndSync(transferAmount)admin function — pass 0 for one-time migration sync after upgrade, or an amount to recover excess tokens before syncing.__gapfrom 46 to 45 to account for the new storage variable.Test plan
internalCashtracking.internalCashsupport.donationAttack.ts— validates that all BSC mainnet core pool markets are vulnerable before upgrade and immune after, including mint/redeem operations post-upgrade.vTokenStorageChecks.ts— validates storage layout and__gapaccounting for the newinternalCashslot.sweepTokenAndSyncCash.tsunit tests for thesweepTokenAndSyncfunction.Summary by CodeRabbit
New Features
Bug Fixes
Tests