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.

contracts/test/VBep20MockDelegate.sol (1)

3-7: Remove duplicate IERC20 import.

IERC20 is 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 sweepTokenAndSync function uses a direct require(msg.sender == admin) check. However, other admin functions in the VToken codebase use the Access Control Manager pattern via ensureAllowed() (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: findBalanceSlot and setTokenBalance are duplicated in donationAttack.ts.

Both findBalanceSlot and setTokenBalance helper functions are identical to those in tests/hardhat/Fork/donationAttack.ts. Consider extracting them to tests/hardhat/Fork/utils.ts for 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.reverted for 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: Early return exits after first successful market.

The return statement 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.reverted may 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`.

tests/hardhat/Fork/vTokenStorageChecks.ts (1)

67-94: Cache discovered balance slots to reduce fork-test cost/flakiness.

findBalanceSlot is 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 with getBalanceSlotCached(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.