Merge pull request #664 from VenusProtocol/feat/VPD-808

[VPD-808] Add internal cash to prevent donation attack

Author: fred-venus

audits/177_Venus VBep20 Donation Patch Final.pdf

@@ -4,7 +4,6 @@ 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 { InterestRateModelV8 } from "../../InterestRateModels/InterestRateModelV8.sol";
 import { VBep20Interface, VTokenInterface } from "./VTokenInterfaces.sol";
@@ -214,6 +213,7 @@ contract VBep20 is VToken, VBep20Interface {
      * @dev Similar to ERC-20 transfer, but handles tokens that have transfer fees.
      *      This function returns the actual amount received,
      *      which may be less than `amount` if there is a fee attached to the transfer.
+     *      Increments `internalCash` by the actual amount received.
      * @param from Sender of the underlying tokens
      * @param amount Amount of underlying to transfer
      * @return Actual amount received
@@ -223,26 +223,49 @@ contract VBep20 is VToken, VBep20Interface {
         uint256 balanceBefore = token.balanceOf(address(this));
         token.safeTransferFrom(from, address(this), amount);
         uint256 balanceAfter = token.balanceOf(address(this));
+        uint256 actualAmount = balanceAfter - balanceBefore;
+        internalCash += actualAmount;
         // Return the amount that was *actually* transferred
-        return balanceAfter - balanceBefore;
+        return actualAmount;
     }
 
     /**
-     * @dev Just a regular ERC-20 transfer, reverts on failure
+     * @dev Just a regular ERC-20 transfer, reverts on failure.
+     *      Decrements `internalCash` by `amount` before transferring.
      * @param to Receiver of the underlying tokens
      * @param amount Amount of underlying to transfer
      */
     function doTransferOut(address payable to, uint256 amount) internal virtual override {
+        internalCash -= amount;
         IERC20 token = IERC20(underlying);
         token.safeTransfer(to, amount);
     }
 
     /**
-     * @notice Gets balance of this contract in terms of the underlying
-     * @dev This excludes the value of the current message, if any
-     * @return The quantity of underlying tokens owned by this contract
+     * @notice Gets the tracked internal cash balance of this contract
+     * @dev Returns `internalCash` rather than the actual token balance, making it immune to donation attacks.
+     * @return The internally tracked cash balance of underlying tokens
      */
     function getCashPrior() internal view override returns (uint) {
-        return IERC20(underlying).balanceOf(address(this));
+        return internalCash;
+    }
+
+    /**
+     * @notice Transfer excess tokens to caller and sync internalCash with actual balance
+     * @dev Admin-only. For migration: pass 0 (just syncs). For sweep: pass the excess amount.
+     *      Transfers `transferAmount` of underlying to msg.sender, then sets internalCash = balanceOf(address(this)).
+     * @param transferAmount Amount of underlying to transfer to msg.sender before syncing
+     */
+    function sweepTokenAndSync(uint256 transferAmount) external {
+        require(msg.sender == admin);
+
+        if (transferAmount > 0) {
+            IERC20(underlying).safeTransfer(msg.sender, transferAmount);
+            emit TokenSwept(msg.sender, transferAmount);
+        }
+
+        uint256 oldInternalCash = internalCash;
+        internalCash = IERC20(underlying).balanceOf(address(this));
+        emit CashSynced(oldInternalCash, internalCash);
     }
 }

audits/178_CertiK-REP-final-20260320T090021Z.pdf

@@ -172,12 +172,18 @@ contract VTokenStorage is VTokenStorageBase {
      */
     uint256 public flashLoanAmount;
 
+    /**
+     * @notice Tracked internal cash balance, immune to direct token transfers (donation attacks)
+     * @dev Updated only via doTransferIn/doTransferOut. Must be initialized via sweepTokenAndSync() after upgrade.
+     */
+    uint256 public internalCash;
+
     /**
      * @dev This empty reserved space is put in place to allow future versions to add new
      * variables without shifting down storage in the inheritance chain.
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
-    uint256[46] private __gap;
+    uint256[45] private __gap;
 }
 
 abstract contract VTokenInterface is VTokenStorage {
@@ -320,6 +326,16 @@ abstract contract VTokenInterface is VTokenStorage {
         uint256 protocolFee
     );
 
+    /**
+     * @notice Event emitted when internalCash is synced with actual token balance
+     */
+    event CashSynced(uint256 oldInternalCash, uint256 newInternalCash);
+
+    /**
+     * @notice Event emitted when excess tokens are swept by admin
+     */
+    event TokenSwept(address indexed recipient, uint256 amount);
+
     /**
      * @notice Event emitted when flashLoan fee mantissa is updated
      */

contracts/Tokens/VTokens/VBep20.sol

@@ -206,4 +206,8 @@ contract EvilXToken is VBep20Delegate {
     function harnessCallBorrowAllowed(uint amount) public returns (uint) {
         return comptroller.borrowAllowed(address(this), msg.sender, amount);
     }
+
+    function harnessSetInternalCash(uint256 _internalCash) public {
+        internalCash = _internalCash;
+    }
 }

contracts/Tokens/VTokens/VTokenInterfaces.sol

@@ -166,6 +166,10 @@ contract VBep20Harness is VBep20Immutable {
     function harnessCallBorrowAllowed(uint amount) public returns (uint) {
         return comptroller.borrowAllowed(address(this), msg.sender, amount);
     }
+
+    function harnessSetInternalCash(uint256 _internalCash) public {
+        internalCash = _internalCash;
+    }
 }
 
 contract VBep20Scenario is VBep20Immutable {
@@ -415,6 +419,10 @@ contract VBep20DelegateHarness is VBep20Delegate {
     function harnessCallBorrowAllowed(uint amount) public returns (uint) {
         return comptroller.borrowAllowed(address(this), msg.sender, amount);
     }
+
+    function harnessSetInternalCash(uint256 _internalCash) public {
+        internalCash = _internalCash;
+    }
 }
 
 contract VBep20DelegateScenario is VBep20Delegate {

contracts/test/EvilXToken.sol

@@ -4,7 +4,6 @@ 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";
 import { InterestRateModelV8 } from "../InterestRateModels/InterestRateModelV8.sol";
 import { VBep20Interface, VTokenInterface } from "../Tokens/VTokens/VTokenInterfaces.sol";
@@ -182,20 +181,20 @@ contract VBep20MockDelegate is VToken, VBep20Interface {
     /*** Safe Token ***/
 
     /**
-     * @notice Gets balance of this contract in terms of the underlying
-     * @dev This excludes the value of the current message, if any
-     * @return The quantity of underlying tokens owned by this contract
+     * @notice Gets the tracked internal cash balance of this contract
+     * @dev Returns `internalCash` rather than the actual token balance, making it immune to donation attacks.
+     * @return The internally tracked cash balance of underlying tokens
      */
     function getCashPrior() internal view override returns (uint) {
-        IERC20 token = IERC20(underlying);
-        return token.balanceOf(address(this));
+        return internalCash;
     }
 
     /**
      * @dev Similar to EIP20 transfer, except it handles a False result from `transferFrom` and reverts in that case.
      *      This will revert due to insufficient balance or insufficient allowance.
      *      This function returns the actual amount received,
      *      which may be less than `amount` if there is a fee attached to the transfer.
+     *      Increments `internalCash` by the actual amount received.
      *
      *      Note: This wrapper safely handles non-standard BEP-20 tokens that do not return a value.
      *            See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
@@ -206,20 +205,28 @@ contract VBep20MockDelegate is VToken, VBep20Interface {
         token.safeTransferFrom(from, address(this), amount);
         // Calculate the amount that was *actually* transferred
         uint balanceAfter = IERC20(underlying).balanceOf(address(this));
-        return balanceAfter - balanceBefore;
+        uint actualAmount = balanceAfter - balanceBefore;
+        internalCash += actualAmount;
+        return actualAmount;
     }
 
     /**
      * @dev Similar to EIP20 transfer, except it handles a False success from `transfer` and returns an explanatory
      *      error code rather than reverting. If caller has not called checked protocol's balance, this may revert due to
      *      insufficient cash held in this contract. If caller has checked protocol's balance prior to this call, and verified
      *      it is >= amount, this should not revert in normal conditions.
+     *      Decrements `internalCash` by `amount` before transferring.
      *
      *      Note: This wrapper safely handles non-standard BEP-20 tokens that do not return a value.
      *            See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
      */
     function doTransferOut(address payable to, uint amount) internal override {
+        internalCash -= amount;
         IERC20 token = IERC20(underlying);
         token.safeTransfer(to, amount);
     }
+
+    function harnessSetInternalCash(uint256 _internalCash) public {
+        internalCash = _internalCash;
+    }
 }

contracts/test/VBep20Harness.sol

@@ -70440,7 +70440,7 @@
       ]
     },
     "VBep20Delegate": {
-      "address": "0x1be1CE8352328278Ac4e0488436c0f1607282550",
+      "address": "0xb25b57599BA969c4829699F7E4Fc4076D14745E1",
       "abi": [
         {
           "inputs": [],
@@ -70597,6 +70597,25 @@
           "name": "Borrow",
           "type": "event"
         },
+        {
+          "anonymous": false,
+          "inputs": [
+            {
+              "indexed": false,
+              "internalType": "uint256",
+              "name": "oldInternalCash",
+              "type": "uint256"
+            },
+            {
+              "indexed": false,
+              "internalType": "uint256",
+              "name": "newInternalCash",
+              "type": "uint256"
+            }
+          ],
+          "name": "CashSynced",
+          "type": "event"
+        },
         {
           "anonymous": false,
           "inputs": [
@@ -71072,6 +71091,25 @@
           "name": "ReservesReduced",
           "type": "event"
         },
+        {
+          "anonymous": false,
+          "inputs": [
+            {
+              "indexed": true,
+              "internalType": "address",
+              "name": "recipient",
+              "type": "address"
+            },
+            {
+              "indexed": false,
+              "internalType": "uint256",
+              "name": "amount",
+              "type": "uint256"
+            }
+          ],
+          "name": "TokenSwept",
+          "type": "event"
+        },
         {
           "anonymous": false,
           "inputs": [
@@ -71844,6 +71882,19 @@
           "stateMutability": "view",
           "type": "function"
         },
+        {
+          "inputs": [],
+          "name": "internalCash",
+          "outputs": [
+            {
+              "internalType": "uint256",
+              "name": "",
+              "type": "uint256"
+            }
+          ],
+          "stateMutability": "view",
+          "type": "function"
+        },
         {
           "inputs": [],
           "name": "isFlashLoanEnabled",
@@ -72279,6 +72330,19 @@
           "stateMutability": "view",
           "type": "function"
         },
+        {
+          "inputs": [
+            {
+              "internalType": "uint256",
+              "name": "transferAmount",
+              "type": "uint256"
+            }
+          ],
+          "name": "sweepTokenAndSync",
+          "outputs": [],
+          "stateMutability": "nonpayable",
+          "type": "function"
+        },
         {
           "inputs": [],
           "name": "symbol",