[REF ONLY] Add BadDebtHelper for atomic bad debt repayment (VIP-604)#666
[REF ONLY] Add BadDebtHelper for atomic bad debt repayment (VIP-604)#666Debugger022 wants to merge 20 commits intodevelopfrom
Conversation
Helper contract that accepts temporary vTHE admin to sweep, repay bad debt across 19 tokens + BNB, and return admin to the timelock in a single atomic transaction.
Verifies end-to-end execution: THE sweep/repay/transfer, BEP20 repayments across 17 markets, BNB repayments, and admin handback.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ 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 a new BadDebtHelper contract, deploy script, artifacts, and a mainnet-fork test. The helper (callable only by NORMAL_TIMELOCK) performs three sequential recovery phases: vTHE admin/sweep/repay, BEP20 token repay loop with allowance handling, and native BNB repay/refund; also adds sweep and receive functions. Changes
Sequence DiagramsequenceDiagram
actor Timelock as NORMAL_TIMELOCK
participant Helper as BadDebtHelper
participant vTHE
participant BEP20 as BEP20_Markets
participant vBNB
participant Treasury
Timelock->>Helper: execute() {value: BNB}
rect rgba(100,150,200,0.5)
Note over Helper,vTHE: Phase 1 — THE repayment
Helper->>vTHE: _acceptAdmin()
Helper->>vTHE: sweepToken(THE)
Helper->>vTHE: approve(THE, amount)
loop THE borrowers
Helper->>vTHE: repayBorrowBehalf(borrower)
end
Helper->>vTHE: sweepToken(THE)
Helper->>Helper: transfer remaining THE -> THE_TARGET_RECEIVER
Helper->>vTHE: _setPendingAdmin(NORMAL_TIMELOCK)
end
rect rgba(150,200,100,0.5)
Note over Helper,BEP20: Phase 2 — BEP20 repayments
loop for each underlying token
Helper->>Token: approve(underlying, MAX) (once)
loop borrowers for token
Helper->>vToken: repayBorrowBehalf(borrower)
end
Helper->>Token: approve(underlying, 0)
Helper->>Timelock: transfer remaining tokens
end
end
rect rgba(200,150,100,0.5)
Note over Helper,vBNB: Phase 3 — BNB repayments
loop BNB borrowers
Helper->>vBNB: repayBorrowBehalf{value: debt}(borrower)
end
Helper->>Timelock: refund remaining BNB
Helper->>Treasury: transfer remaining ERC20 (non-THE)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
Drop upgradeable proxy — unnecessary for a one-shot helper. Fix approval ceiling (use type(uint256).max) and stale event emission (use balance diff). Remove redundant balanceOf call in _repayToken for gas savings.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
@coderabbitai review this now^ |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Tokens/VTokens/BadDebtHelper.sol`:
- Around line 355-361: Replace uses of vToken.borrowBalanceStored(borrower) with
vToken.borrowBalanceCurrent(borrower) in the BadDebtHelper repayment logic (both
the block at the shown lines and the similar block at 381-386) so the repay
branch uses the up-to-date debt before deciding repayAmount and calling
repayBorrowBehalf; i.e., call borrowBalanceCurrent, return if zero, compute
balance = underlying.balanceOf(address(this)), then compute repayAmount based on
the current debt (debt <= balance ? type(uint256).max : balance) and call
repayBorrowBehalf(borrower, repayAmount).
In `@deploy/019-deploy-bad-debt-helper.ts`:
- Line 24: The deploy script currently sets func.skip = skipRemoteNetworks(),
which still permits bsctestnet and hardhat; change the skip predicate so the
script runs only on BSC mainnet (e.g., require network name equals "bscmainnet")
and optionally allow local forked hardhat runs (e.g., allow when network is
"hardhat" and FORK env is set); update the assignment to func.skip to check for
those exact network names instead of calling skipRemoteNetworks(), so this
helper cannot run on testnets by accident.
In `@tests/hardhat/Fork/badDebtHelper.ts`:
- Around line 421-433: The current checks only ensure timelock balances didn't
decrease but don't confirm refunds went to NORMAL_TIMELOCK; update the BEP20
loop (using BEP20_MARKETS, ERC20__factory, NORMAL_TIMELOCK, preTimelockBalances)
to assert that postTimelockBalance is strictly greater than preTimelockBalance
(i.e., a positive refund delta) rather than just >=; likewise for the BNB path
(the assertions around the helper/BNB zero-balance at lines ~464-467),
deliberately overfund the BNB test by a known surplus and assert that
NORMAL_TIMELOCK's native balance increased by the expected refund amount (not
just that the helper balance is zero) so the refund recipient is validated.
- Around line 322-325: When findBalanceSlot(market.underlying) returns null for
a market, do not silently continue; either throw or record the skipped market so
the later repayment assertions do not include it. Update the block around
findBalanceSlot/market.name to (a) throw a descriptive error (including
market.name and market.underlying) so the suite fails fast, or (b) push market
identifier into a skippedMarkets array and modify the later repayment assertion
logic (the code that iterates borrowers to verify post-debt balances) to ignore
any markets present in skippedMarkets. Ensure the chosen approach updates all
references to repayment assertions so they only evaluate markets that were
actually funded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a74402af-5ee0-47c0-8b3e-c243a8505d72
📒 Files selected for processing (7)
contracts/Tokens/VTokens/BadDebtHelper.soldeploy/019-deploy-bad-debt-helper.tsdeployments/bscmainnet.jsondeployments/bscmainnet/BadDebtHelper.jsondeployments/bscmainnet/solcInputs/cdb75d619363c63d9052ded803cba631.jsondeployments/bscmainnet_addresses.jsontests/hardhat/Fork/badDebtHelper.ts
| uint256 debt = vToken.borrowBalanceStored(borrower); | ||
| if (debt == 0) return; | ||
|
|
||
| uint256 balance = underlying.balanceOf(address(this)); | ||
| uint256 repayAmount = debt <= balance ? type(uint256).max : balance; | ||
|
|
||
| uint256 err = vToken.repayBorrowBehalf(borrower, repayAmount); |
There was a problem hiding this comment.
borrowBalanceStored() makes the max repayment path unsafe.
Lines 355-361 and 381-386 branch on a stale stored debt value, then call repayBorrowBehalf(), which accrues again. If the helper balance only matches the stored snapshot, the type(uint256).max path can still revert once the live debt is slightly higher, which defeats the timelock-delay tolerance this helper is trying to provide. borrowBalanceCurrent() is already available on the shared interface and should drive both branches.
Patch sketch
- uint256 debt = vToken.borrowBalanceStored(borrower);
+ uint256 debt = vToken.borrowBalanceCurrent(borrower);
if (debt == 0) return;
uint256 balance = underlying.balanceOf(address(this));
+ if (balance == 0) return;
uint256 repayAmount = debt <= balance ? type(uint256).max : balance;
@@
- uint256 debt = vToken.borrowBalanceStored(borrower);
+ uint256 debt = vToken.borrowBalanceCurrent(borrower);
if (debt == 0) return;
uint256 balanceBefore = THE.balanceOf(address(this));
- uint256 err = vToken.repayBorrowBehalf(borrower, type(uint256).max);
+ if (balanceBefore == 0) return;
+ uint256 repayAmount = debt <= balanceBefore ? type(uint256).max : balanceBefore;
+ uint256 err = vToken.repayBorrowBehalf(borrower, repayAmount);Also applies to: 381-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/Tokens/VTokens/BadDebtHelper.sol` around lines 355 - 361, Replace
uses of vToken.borrowBalanceStored(borrower) with
vToken.borrowBalanceCurrent(borrower) in the BadDebtHelper repayment logic (both
the block at the shown lines and the similar block at 381-386) so the repay
branch uses the up-to-date debt before deciding repayAmount and calling
repayBorrowBehalf; i.e., call borrowBalanceCurrent, return if zero, compute
balance = underlying.balanceOf(address(this)), then compute repayAmount based on
the current debt (debt <= balance ? type(uint256).max : balance) and call
repayBorrowBehalf(borrower, repayAmount).
There was a problem hiding this comment.
This is a false positive. borrowBalanceStored is intentionally used here rather than borrowBalanceCurrent for the following reasons:
-
repayBorrowBehalfalready callsaccrueInterest()internally (viarepayBorrowBehalfInternal), so the actual repayment always operates on the fully up-to-date debt. -
The
borrowBalanceStoredcall is only used for pre-checks — the zero-debt early return and thetype(uint256).maxvsbalancedecision. Any interest delta between the stale read
and the accrued value insiderepayBorrowBehalfis at most one block of interest within the same transaction, which is negligible. -
Using
borrowBalanceCurrentwould redundantly accrue interest twice — once explicitly here, and once insiderepayBorrowBehalf— wasting gas for no practical benefit. -
The helper is funded with margin from the timelock to absorb any tiny interest accrual difference.
No changes needed.
There was a problem hiding this comment.
@Debugger022, you're right — the reasoning is sound on all four points. Since repayBorrowBehalf already calls accrueInterest() internally, the stored value is only used as a lightweight pre-check for the branching decision, and any single-block interest delta is covered by the funding margin. Calling borrowBalanceCurrent would just burn extra gas with a redundant accrual. I'll retract this finding.
✏️ Learnings added
Learnt from: Debugger022
Repo: VenusProtocol/venus-protocol PR: 666
File: contracts/Tokens/VTokens/BadDebtHelper.sol:355-361
Timestamp: 2026-03-25T14:48:13.207Z
Learning: In BadDebtHelper.sol (contracts/Tokens/VTokens/BadDebtHelper.sol), `borrowBalanceStored` is intentionally used (instead of `borrowBalanceCurrent`) for the zero-debt early-return check and the `type(uint256).max` vs `balance` branching decision. This is by design: `repayBorrowBehalf` already calls `accrueInterest()` internally (via `repayBorrowBehalfInternal`), so the actual repayment always operates on the fully up-to-date debt. Using `borrowBalanceCurrent` would redundantly accrue interest twice and waste gas. The helper is also funded with margin to absorb any tiny single-block interest delta. Do not flag this as an issue in future reviews.
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.
|
|
||
| func.id = "bad_debt_helper_deploy"; | ||
| func.tags = ["BadDebtHelper"]; | ||
| func.skip = skipRemoteNetworks(); |
There was a problem hiding this comment.
Narrow this deploy to mainnet-only.
Line 24 reuses skipRemoteNetworks(), which still allows bsctestnet and hardhat. This helper is wired to BSC mainnet markets/borrowers, so a testnet deployment would produce a contract that looks valid but points at the wrong pool state and timelock. Please gate this script to bscmainnet only, or explicitly to forked hardhat if you need local smoke deployments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/019-deploy-bad-debt-helper.ts` at line 24, The deploy script currently
sets func.skip = skipRemoteNetworks(), which still permits bsctestnet and
hardhat; change the skip predicate so the script runs only on BSC mainnet (e.g.,
require network name equals "bscmainnet") and optionally allow local forked
hardhat runs (e.g., allow when network is "hardhat" and FORK env is set); update
the assignment to func.skip to check for those exact network names instead of
calling skipRemoteNetworks(), so this helper cannot run on testnets by accident.
| const slot = await findBalanceSlot(market.underlying); | ||
| if (slot === null) { | ||
| console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`); | ||
| continue; |
There was a problem hiding this comment.
Don't silently skip a market that the suite still expects to repay.
If Line 323 returns null and Line 325 continues, the later debt assertions still require every non-zero borrower in that market to have lower post-debt. That turns a known setup failure into a downstream, misleading test failure. Fail fast here, or record skipped markets and exclude them from the repayment assertions.
Suggested fix
const slot = await findBalanceSlot(market.underlying);
if (slot === null) {
- console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`);
- continue;
+ throw new Error(
+ `Could not find balance slot for ${market.name}; cannot fund helper or verify this market on the fork`,
+ );
}📝 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 slot = await findBalanceSlot(market.underlying); | |
| if (slot === null) { | |
| console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`); | |
| continue; | |
| const slot = await findBalanceSlot(market.underlying); | |
| if (slot === null) { | |
| throw new Error( | |
| `Could not find balance slot for ${market.name}; cannot fund helper or verify this market on the fork`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 322 - 325, When
findBalanceSlot(market.underlying) returns null for a market, do not silently
continue; either throw or record the skipped market so the later repayment
assertions do not include it. Update the block around
findBalanceSlot/market.name to (a) throw a descriptive error (including
market.name and market.underlying) so the suite fails fast, or (b) push market
identifier into a skippedMarkets array and modify the later repayment assertion
logic (the code that iterates borrowers to verify post-debt balances) to ignore
any markets present in skippedMarkets. Ensure the chosen approach updates all
references to repayment assertions so they only evaluate markets that were
actually funded.
tests/hardhat/Fork/badDebtHelper.ts
Outdated
| it("should return unused BEP20 tokens to timelock", async () => { | ||
| // For each token, timelock should have gotten back any excess | ||
| for (const market of BEP20_MARKETS) { | ||
| const token = ERC20__factory.connect(market.underlying, ethers.provider); | ||
| const postTimelockBalance = await token.balanceOf(NORMAL_TIMELOCK); | ||
| const preTimelockBalance = preTimelockBalances[market.underlying]; | ||
|
|
||
| // Timelock balance should be >= pre-balance (got back unused tokens) | ||
| expect(postTimelockBalance).to.be.gte( | ||
| preTimelockBalance, | ||
| `Timelock ${market.name} balance should not decrease`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
These assertions don't actually verify the refund recipient.
Because Line 320 intentionally overfunds each BEP20 market by 2x, the refund path should be exercised for every funded market. But Lines 428-432 only check that the timelock did not lose tokens, which still passes if the surplus is sent somewhere other than NORMAL_TIMELOCK. Lines 464-467 have the same blind spot for BNB: zero helper balance only proves the funds left the helper. Please assert a positive/refund delta to the timelock for BEP20, and deliberately overfund the BNB path by a known surplus so the BNB refund recipient is validated too.
Also applies to: 464-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 421 - 433, The current
checks only ensure timelock balances didn't decrease but don't confirm refunds
went to NORMAL_TIMELOCK; update the BEP20 loop (using BEP20_MARKETS,
ERC20__factory, NORMAL_TIMELOCK, preTimelockBalances) to assert that
postTimelockBalance is strictly greater than preTimelockBalance (i.e., a
positive refund delta) rather than just >=; likewise for the BNB path (the
assertions around the helper/BNB zero-balance at lines ~464-467), deliberately
overfund the BNB test by a known surplus and assert that NORMAL_TIMELOCK's
native balance increased by the expected refund amount (not just that the helper
balance is zero) so the refund recipient is validated.
| // Part 3: Native BNB bad debt repayments | ||
| // ══════════════════════════════════════════════════════════ | ||
|
|
||
| function _repayBNB() internal { |
There was a problem hiding this comment.
cant we
uint256 debt = IVBnb(address(V_BNB)).borrowBalanceCurrent(ACCOUNT_4);
V_BNB.repayBorrowBehalf{value: debt}(ACCOUNT_4);
There was a problem hiding this comment.
984237b - Replaced 5 hardcoded BNB_REPAY_* constants with dynamic borrowBalanceCurrent calls
via a new _repayBNBFor helper
| event BadDebtRepaid(address indexed vToken, address indexed borrower, uint256 repayAmount); | ||
| event RemainingTHETransferred(address indexed receiver, uint256 amount); | ||
| event TokensReturnedToTimelock(address indexed token, uint256 amount); | ||
|
|
There was a problem hiding this comment.
pls also add a sweep function just in case
function sweep(token, amt){
require(msg.sender == NORMAL_TIMELOCK, "only timelock");
if (token == addr(0) -> bnb withdraw
otherwise erc20 withdraw
}
There was a problem hiding this comment.
0c633af Added timelock-only sweep(address, uint256) supporting both BNB and ERC20
| _repayTHE(); | ||
| _repayBEP20(); | ||
| _repayBNB(); | ||
| } |
There was a problem hiding this comment.
lets have last step here, walk through all the tokens involved (except THE which will flow to recipient)
for each, trasnfert token.balanceOf(addr(this)) to treasury including bnb
There was a problem hiding this comment.
this contract should be completely empty before exit
There was a problem hiding this comment.
8fbb8c8 — sweeping all 17 BEP20 tokens + BNB back to timelock
Replace hardcoded BNB_REPAY_* constants with dynamic borrowBalanceCurrent calls to fetch exact debt amounts on-chain.
Add timelock-only sweep function to recover any stuck BNB or ERC20 tokens from the contract.
…ecution Add _returnAllRemaining() as final step in execute() to sweep all 17 BEP20 tokens and any remaining BNB back to the Normal Timelock, ensuring the contract is completely empty after execution.
Compute BNB funding dynamically from on-chain borrowBalanceStored instead of using hardcoded amounts, matching the contract change to use borrowBalanceCurrent.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/hardhat/Fork/badDebtHelper.ts (1)
317-321:⚠️ Potential issue | 🟡 MinorSilent skip can cause misleading downstream test failures.
If
findBalanceSlotreturnsnull, the market is skipped without funding, but the later assertions (lines 396-410) still iterate over allBEP20_MARKETSand expect debt reduction. This causes a confusing test failure instead of a clear setup error.Suggested fix: Fail fast on setup failure
const slot = await findBalanceSlot(market.underlying); if (slot === null) { - console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`); - continue; + throw new Error( + `Could not find balance slot for ${market.name} (${market.underlying}), cannot fund helper`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 317 - 321, The setup silently skips funding when findBalanceSlot(market.underlying) returns null (slot === null), which leads to confusing downstream assertion failures over BEP20_MARKETS; change this to fail fast by throwing or asserting with a clear error (including market.name and underlying) inside the loop (the block that currently checks slot === null) so tests abort immediately on setup failure instead of continuing to the debt-reduction assertions.
🧹 Nitpick comments (3)
contracts/Tokens/VTokens/BadDebtHelper.sol (1)
424-434:_repayIfDebtfor THE always usestype(uint256).maxwithout checking available balance.Unlike
_repayToken(lines 362-366) which conditionally usestype(uint256).maxonly whendebt <= balance,_repayIfDebtunconditionally passestype(uint256).maxtorepayBorrowBehalf. If the helper's THE balance is insufficient to cover a borrower's full debt (after interest accrual), the call may revert.This appears intentional since THE is swept from vTHE first (line 181-182), so the helper should have sufficient THE. However, if the swept amount is unexpectedly low, the entire transaction would revert rather than performing partial repayment.
Optional: Add partial repayment support for consistency
function _repayIfDebt(IVBep20 vToken, address borrower) internal { uint256 debt = vToken.borrowBalanceStored(borrower); if (debt == 0) return; uint256 balanceBefore = THE.balanceOf(address(this)); - uint256 err = vToken.repayBorrowBehalf(borrower, type(uint256).max); + uint256 repayAmount = debt <= balanceBefore ? type(uint256).max : balanceBefore; + if (repayAmount == 0) return; + uint256 err = vToken.repayBorrowBehalf(borrower, repayAmount); if (err != NO_ERROR) revert VTokenError(address(vToken), err); uint256 actualRepaid = balanceBefore - THE.balanceOf(address(this)); emit BadDebtRepaid(address(vToken), borrower, actualRepaid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/Tokens/VTokens/BadDebtHelper.sol` around lines 424 - 434, The _repayIfDebt function currently always calls vToken.repayBorrowBehalf(borrower, type(uint256).max) which can revert if THE balance is insufficient; change it to compute repayAmount = min(debt, balanceBefore) where balanceBefore = THE.balanceOf(address(this)), skip the repay if repayAmount == 0, call vToken.repayBorrowBehalf(borrower, repayAmount), check err and revert on error (VTokenError), and compute actualRepaid using the difference of THE balances after the call before emitting BadDebtRepaid; reference functions/variables: _repayIfDebt, vToken.repayBorrowBehalf, THE.balanceOf, debt variable, and BadDebtRepaid event.tests/hardhat/Fork/badDebtHelper.ts (2)
336-345: Test BNB funding usesborrowBalanceStoredwhile contract usesborrowBalanceCurrent.The test computes
totalBNBusingborrowBalanceStored(line 339) and multiplies by 2 for margin. However, the contract's_repayBNBForusesborrowBalanceCurrent(line 341 in contract) which accrues interest and returns a higher value. The 2x margin should be sufficient to cover single-block interest accrual, but this subtle difference is worth noting for clarity.Optional: Use borrowBalanceCurrent in test for consistency
for (const borrower of BNB_BORROWERS) { - const debt = await vBNB.borrowBalanceStored(borrower); + const debt = await vBNB.borrowBalanceCurrent(borrower); totalBNB = totalBNB.add(debt); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 336 - 345, The test computes totalBNB using vBNB.borrowBalanceStored but the contract repays using borrowBalanceCurrent (which includes accrued interest); update the loop to call vBNB.borrowBalanceCurrent for each borrower (or use callStatic if necessary) when summing totalBNB so the test funding matches the contract’s current-balance semantics, and keep the existing multiply-by-2 margin before calling helper.connect(timelock).execute({ value: totalBNB }).
477-494: Consider adding test coverage for thesweep()function.The
sweep()function was added for emergency recovery but isn't tested in this suite. Consider adding tests to verify:
- Timelock can sweep stuck tokens
- Timelock can sweep stuck BNB
- Non-timelock callers are rejected
Example test cases
describe("sweep function", () => { it("should allow timelock to sweep stuck tokens", async () => { // Fund helper with some token // Call sweep from timelock // Verify token transferred to timelock }); it("should allow timelock to sweep stuck BNB", async () => { // Send BNB to helper // Call sweep(address(0), amount) from timelock // Verify BNB transferred to timelock }); it("should revert if called by non-timelock", async () => { const [randomSigner] = await ethers.getSigners(); await expect(helper.connect(randomSigner).sweep(THE, 0)) .to.be.revertedWith("only timelock"); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 477 - 494, Add a new "sweep function" test block that covers three cases: (1) timelock can sweep stuck ERC20 tokens — fund helper with a small THE/ERC20 balance via ERC20__factory.connect(THE, ...).transfer(helper.address, amount) or mint, then call helper.connect(timelockSigner).sweep(THE, amount) and assert the timelock balance increased and helper balance decreased; (2) timelock can sweep BNB — send ETH to helper via ethers.provider.getSigner().sendTransaction({to: helper.address, value}), call helper.connect(timelockSigner).sweep(ethers.constants.AddressZero, amount) and assert timelock ETH balance increased; (3) non-timelock callers are rejected — pick a random signer and assert helper.connect(randomSigner).sweep(THE, amount) reverts with "only timelock". Use existing symbols helper, sweep(), THE, timelockSigner (or timelock address), ERC20__factory, and ethers.provider for locating code.
🤖 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/badDebtHelper.ts`:
- Around line 317-321: The setup silently skips funding when
findBalanceSlot(market.underlying) returns null (slot === null), which leads to
confusing downstream assertion failures over BEP20_MARKETS; change this to fail
fast by throwing or asserting with a clear error (including market.name and
underlying) inside the loop (the block that currently checks slot === null) so
tests abort immediately on setup failure instead of continuing to the
debt-reduction assertions.
---
Nitpick comments:
In `@contracts/Tokens/VTokens/BadDebtHelper.sol`:
- Around line 424-434: The _repayIfDebt function currently always calls
vToken.repayBorrowBehalf(borrower, type(uint256).max) which can revert if THE
balance is insufficient; change it to compute repayAmount = min(debt,
balanceBefore) where balanceBefore = THE.balanceOf(address(this)), skip the
repay if repayAmount == 0, call vToken.repayBorrowBehalf(borrower, repayAmount),
check err and revert on error (VTokenError), and compute actualRepaid using the
difference of THE balances after the call before emitting BadDebtRepaid;
reference functions/variables: _repayIfDebt, vToken.repayBorrowBehalf,
THE.balanceOf, debt variable, and BadDebtRepaid event.
In `@tests/hardhat/Fork/badDebtHelper.ts`:
- Around line 336-345: The test computes totalBNB using vBNB.borrowBalanceStored
but the contract repays using borrowBalanceCurrent (which includes accrued
interest); update the loop to call vBNB.borrowBalanceCurrent for each borrower
(or use callStatic if necessary) when summing totalBNB so the test funding
matches the contract’s current-balance semantics, and keep the existing
multiply-by-2 margin before calling helper.connect(timelock).execute({ value:
totalBNB }).
- Around line 477-494: Add a new "sweep function" test block that covers three
cases: (1) timelock can sweep stuck ERC20 tokens — fund helper with a small
THE/ERC20 balance via ERC20__factory.connect(THE, ...).transfer(helper.address,
amount) or mint, then call helper.connect(timelockSigner).sweep(THE, amount) and
assert the timelock balance increased and helper balance decreased; (2) timelock
can sweep BNB — send ETH to helper via
ethers.provider.getSigner().sendTransaction({to: helper.address, value}), call
helper.connect(timelockSigner).sweep(ethers.constants.AddressZero, amount) and
assert timelock ETH balance increased; (3) non-timelock callers are rejected —
pick a random signer and assert helper.connect(randomSigner).sweep(THE, amount)
reverts with "only timelock". Use existing symbols helper, sweep(), THE,
timelockSigner (or timelock address), ERC20__factory, and ethers.provider for
locating code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfce8492-bfb9-4cfa-9cc6-c3457bffaa12
📒 Files selected for processing (2)
contracts/Tokens/VTokens/BadDebtHelper.soltests/hardhat/Fork/badDebtHelper.ts
| function _returnIfBalance(IERC20Upgradeable token) internal { | ||
| uint256 balance = token.balanceOf(address(this)); | ||
| if (balance > 0) { | ||
| token.safeTransfer(NORMAL_TIMELOCK, balance); |
|
|
||
| uint256 remainingBNB = address(this).balance; | ||
| if (remainingBNB > 0) { | ||
| (bool success, ) = NORMAL_TIMELOCK.call{ value: remainingBNB }(""); |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/hardhat/Fork/badDebtHelper.ts (1)
318-322:⚠️ Potential issue | 🟡 MinorSilently skipping markets can cause misleading test failures.
If
findBalanceSlotreturnsnull, the test silently skips funding that market but the later debt assertions at lines 396-411 still check those borrowers. A setup failure becomes a misleading downstream test failure.🔧 Suggested fix: fail fast
const slot = await findBalanceSlot(market.underlying); if (slot === null) { - console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`); - continue; + throw new Error( + `Could not find balance slot for ${market.name} (${market.underlying}); cannot fund helper`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 318 - 322, The code currently logs and continues when findBalanceSlot(market.underlying) returns null, which silently skips funding a market and leads to misleading downstream debt assertions; change this to fail fast by throwing or asserting when slot is null (e.g., replace the console warning + continue with a thrown Error or assert that includes market.name and market.underlying), so the test setup stops immediately and reports which market failed to find a balance slot; locate this in the funding loop where findBalanceSlot is called and modify the control flow accordingly.
🧹 Nitpick comments (2)
tests/hardhat/Fork/badDebtHelper.ts (2)
425-438: Consider strengthening refund assertions.The current check (
postTimelockBalance >= preTimelockBalance) verifies the timelock didn't lose tokens but doesn't confirm refunds actually went there vs. somewhere else. Since_finalizeTokensends unused tokens toNORMAL_TIMELOCK, consider asserting a positive delta for markets with excess funding:- // Timelock balance should be >= pre-balance (got back unused tokens) - expect(postTimelockBalance).to.be.gte( - preTimelockBalance, - `Timelock ${market.name} balance should not decrease`, - ); + // Timelock should have received refunded surplus + expect(postTimelockBalance).to.be.gt( + preTimelockBalance, + `Timelock ${market.name} balance should increase (received refund)`, + );Alternatively, track the expected refund amount (fundAmount - actualRepaid) and assert against it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 425 - 438, The test currently only asserts timelock balances didn't decrease; update the loop in the "should return unused BEP20 tokens to timelock" test to assert that markets which had excess funding actually increased the timelock balance: for each market in BEP20_MARKETS use ERC20__factory.connect(market.underlying, ethers.provider) to get pre and post balances (preTimelockBalances and postTimelockBalance) and assert postTimelockBalance.gt(preTimelockBalance) when the market had excess, or compute the expected refund (e.g., fundAmount - actualRepaid retrieved from the market/funding records) and assert postTimelockBalance.sub(preTimelockBalance).gte(expectedRefund); ensure you reference NORMAL_TIMELOCK, preTimelockBalances, and the logic in _finalizeToken to determine which markets should see a positive delta.
468-471: Consider adding BNB refund recipient verification.The test confirms helper ends with 0 BNB but doesn't verify where unused BNB went. Since the contract sends unused BNB to
NORMAL_TIMELOCK, consider tracking pre/post timelock BNB balance to confirm the refund destination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 468 - 471, Add a verification that unused BNB was forwarded to the NORMAL_TIMELOCK by recording the timelock balance before the helper operation (use ethers.provider.getBalance(NORMAL_TIMELOCK) or the timelock contract address), run the logic under test, then fetch the timelock balance again and assert the postBalance increased accordingly (e.g., postBalance > preBalance or postBalance === preBalance + refundedAmount) in the same test that checks helper.address has 0 BNB; reference helper.address, NORMAL_TIMELOCK, and ethers.provider.getBalance to locate and implement the checks.
🤖 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/Tokens/VTokens/BadDebtHelper.sol`:
- Around line 341-347: The _repayBNBFor function currently attempts to send the
full debt and can revert if the contract BNB balance is insufficient; update it
to mirror the BEP20 path by reading address(this).balance, compute repayAmount =
debt <= balance ? type(uint256).max : balance, and call V_BNB.repayBorrowBehalf{
value: repayAmount }(borrower) so partial repayments are accepted instead of
reverting; also update the emitted BadDebtRepaid event payload to reflect the
actual repaid amount (use repayAmount instead of debt) and keep the initial
early-return when debt == 0.
- Around line 388-423: The event name TokensReturnedToTimelock is misleading
because _returnAllRemaining and _returnIfBalance send funds to TREASURY but emit
TokensReturnedToTimelock; update the contract to declare and emit a
treasury-specific event (e.g. TokensReturnedToTreasury(address indexed token,
uint256 amount)) or replace with a generic event (e.g. TokensReturned(address
indexed token, address indexed recipient, uint256 amount)) and change the emit
calls in _returnAllRemaining and _returnIfBalance to use the new event (also
update any event listeners/tests/ABI consumers to match the new event
signature).
In `@tests/hardhat/Fork/badDebtHelper.ts`:
- Line 15: Remove the unused TREASURY constant from badDebtHelper.ts or use it
in the test assertions; specifically either delete the line defining TREASURY or
add assertions that reference TREASURY to verify remaining tokens are sent to
the treasury (e.g., check token balances/transfers to the address), updating any
helper functions like those that inspect balances or transfer events so they use
the TREASURY symbol.
---
Duplicate comments:
In `@tests/hardhat/Fork/badDebtHelper.ts`:
- Around line 318-322: The code currently logs and continues when
findBalanceSlot(market.underlying) returns null, which silently skips funding a
market and leads to misleading downstream debt assertions; change this to fail
fast by throwing or asserting when slot is null (e.g., replace the console
warning + continue with a thrown Error or assert that includes market.name and
market.underlying), so the test setup stops immediately and reports which market
failed to find a balance slot; locate this in the funding loop where
findBalanceSlot is called and modify the control flow accordingly.
---
Nitpick comments:
In `@tests/hardhat/Fork/badDebtHelper.ts`:
- Around line 425-438: The test currently only asserts timelock balances didn't
decrease; update the loop in the "should return unused BEP20 tokens to timelock"
test to assert that markets which had excess funding actually increased the
timelock balance: for each market in BEP20_MARKETS use
ERC20__factory.connect(market.underlying, ethers.provider) to get pre and post
balances (preTimelockBalances and postTimelockBalance) and assert
postTimelockBalance.gt(preTimelockBalance) when the market had excess, or
compute the expected refund (e.g., fundAmount - actualRepaid retrieved from the
market/funding records) and assert
postTimelockBalance.sub(preTimelockBalance).gte(expectedRefund); ensure you
reference NORMAL_TIMELOCK, preTimelockBalances, and the logic in _finalizeToken
to determine which markets should see a positive delta.
- Around line 468-471: Add a verification that unused BNB was forwarded to the
NORMAL_TIMELOCK by recording the timelock balance before the helper operation
(use ethers.provider.getBalance(NORMAL_TIMELOCK) or the timelock contract
address), run the logic under test, then fetch the timelock balance again and
assert the postBalance increased accordingly (e.g., postBalance > preBalance or
postBalance === preBalance + refundedAmount) in the same test that checks
helper.address has 0 BNB; reference helper.address, NORMAL_TIMELOCK, and
ethers.provider.getBalance to locate and implement the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b1ba532-f309-49b4-92fa-046b9270f7c9
📒 Files selected for processing (2)
contracts/Tokens/VTokens/BadDebtHelper.soltests/hardhat/Fork/badDebtHelper.ts
| function _repayBNBFor(address borrower) internal { | ||
| uint256 debt = IVBnb(address(V_BNB)).borrowBalanceCurrent(borrower); | ||
| if (debt == 0) return; | ||
|
|
||
| V_BNB.repayBorrowBehalf{ value: debt }(borrower); | ||
| emit BadDebtRepaid(address(V_BNB), borrower, debt); | ||
| } |
There was a problem hiding this comment.
BNB repayment lacks partial repayment handling unlike BEP20 path.
_repayBNBFor sends exactly the full debt amount without checking if address(this).balance >= debt. If interest accrual during timelock delay exceeds the 2× funding margin, this will revert. The BEP20 path handles this gracefully:
uint256 repayAmount = debt <= balance ? type(uint256).max : balance;Consider adding similar protection for BNB:
🛡️ Proposed fix to handle insufficient BNB balance
function _repayBNBFor(address borrower) internal {
uint256 debt = IVBnb(address(V_BNB)).borrowBalanceCurrent(borrower);
if (debt == 0) return;
- V_BNB.repayBorrowBehalf{ value: debt }(borrower);
- emit BadDebtRepaid(address(V_BNB), borrower, debt);
+ uint256 balance = address(this).balance;
+ uint256 repayAmount = debt <= balance ? debt : balance;
+ if (repayAmount == 0) return;
+
+ V_BNB.repayBorrowBehalf{ value: repayAmount }(borrower);
+ emit BadDebtRepaid(address(V_BNB), borrower, repayAmount);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/Tokens/VTokens/BadDebtHelper.sol` around lines 341 - 347, The
_repayBNBFor function currently attempts to send the full debt and can revert if
the contract BNB balance is insufficient; update it to mirror the BEP20 path by
reading address(this).balance, compute repayAmount = debt <= balance ?
type(uint256).max : balance, and call V_BNB.repayBorrowBehalf{ value:
repayAmount }(borrower) so partial repayments are accepted instead of reverting;
also update the emitted BadDebtRepaid event payload to reflect the actual repaid
amount (use repayAmount instead of debt) and keep the initial early-return when
debt == 0.
There was a problem hiding this comment.
The current implementation is correct and intentional — vBNB's
repayBorrowBehalf works fundamentally differently from the BEP20
path:
- vBNB accepts native BNB via msg.value and refunds any excess
automatically. There's no type(uint256).max sentinel support — it
uses the literal ETH value sent. The BEP20 sentinel pattern
(type(uint256).max) is specific to transferFrom-based repayments. - borrowBalanceCurrent (line 341) already accrues interest before
returning the debt, so the amount is accurate at execution time —
unlike borrowBalanceStored used in the BEP20 path which can be stale. - Silently partial-repaying would be worse than reverting for an
atomic bad-debt cleanup. If the contract somehow lacks sufficient
BNB, we want a revert rather than leaving unrepaid debt behind
unnoticed. - The contract is funded with ample margin (2x estimated debt in the
VIP), so balance insufficiency is not a realistic scenario.
No changes needed.
There was a problem hiding this comment.
@Debugger022, thank you for the detailed explanation — all four points are well-taken:
type(uint256).maxis atransferFrom-based sentinel specific to ERC20 repayments and has no meaning in the native-BNB path wheremsg.valueis the literal amount sent (and any excess is refunded by vBNB automatically).borrowBalanceCurrentalready performs interest accrual, so the amount passed torepayBorrowBehalfis fresh and accurate.- A silent partial repayment would undermine the atomicity guarantee of the cleanup — a revert is the correct failure mode.
- With a 2× funding margin baked into the VIP, a shortfall is not a realistic scenario.
My suggestion was incorrect. I'll record this for future reviews.
(ᓀ ᓀ)
✏️ Learnings added
Learnt from: Debugger022
Repo: VenusProtocol/venus-protocol PR: 666
File: contracts/Tokens/VTokens/BadDebtHelper.sol:341-347
Timestamp: 2026-03-26T04:16:18.244Z
Learning: In BadDebtHelper.sol (contracts/Tokens/VTokens/BadDebtHelper.sol), the `_repayBNBFor` function intentionally sends the exact `borrowBalanceCurrent` debt as `msg.value` to `V_BNB.repayBorrowBehalf`. This is correct because: (1) vBNB's `repayBorrowBehalf` accepts native BNB via `msg.value` and automatically refunds any excess — the `type(uint256).max` sentinel is only meaningful for ERC20 `transferFrom`-based repayments and does not apply here; (2) `borrowBalanceCurrent` already accrues interest before returning, so the amount is up-to-date at execution time; (3) silent partial repayment is undesirable for an atomic bad-debt cleanup — a revert on underfunding is the intended behavior; (4) the contract is funded with a 2× margin in the VIP, making balance insufficiency unrealistic. Do not flag this as an issue in future reviews.
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/venus-protocol PR: 666
File: contracts/Tokens/VTokens/BadDebtHelper.sol:355-361
Timestamp: 2026-03-25T14:48:13.207Z
Learning: In BadDebtHelper.sol (contracts/Tokens/VTokens/BadDebtHelper.sol), `borrowBalanceStored` is intentionally used (instead of `borrowBalanceCurrent`) for the zero-debt early-return check and the `type(uint256).max` vs `balance` branching decision. This is by design: `repayBorrowBehalf` already calls `accrueInterest()` internally (via `repayBorrowBehalfInternal`), so the actual repayment always operates on the fully up-to-date debt. Using `borrowBalanceCurrent` would redundantly accrue interest twice and waste gas. The helper is also funded with margin to absorb any tiny single-block interest delta. Do not flag this as an issue in future reviews.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/hardhat/Fork/badDebtHelper.ts (3)
317-321:⚠️ Potential issue | 🟠 MajorSilent skip on failed slot finding causes misleading downstream test failures.
When
findBalanceSlot()returnsnull, the market is silently skipped viacontinue, but the later debt assertions (lines 394-411) still run for that market. If the market wasn't funded, the assertions will either fail misleadingly or pass incorrectly depending on the fork state.Fail fast here or track skipped markets to exclude from assertions.
🐛 Suggested fix
const slot = await findBalanceSlot(market.underlying); if (slot === null) { - console.log(` WARNING: Could not find balance slot for ${market.name}, skipping funding`); - continue; + throw new Error( + `Could not find balance slot for ${market.name} (${market.underlying}); cannot fund helper or verify this market on the fork`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 317 - 321, The loop that calls findBalanceSlot(market.underlying) currently logs and continues when slot === null, which lets later debt assertions run against markets that were never funded; change this so failures are explicit: either throw an error when findBalanceSlot returns null to fail fast (so fix by replacing the console warning+continue with a thrown Error referencing the market.name and findBalanceSlot), or maintain a skipped list (e.g., skippedMarkets) when slot === null and then exclude any market in skippedMarkets from the later debt assertion block; reference findBalanceSlot, the slot variable, and the debt assertion logic to implement one of these two behaviors.
467-470:⚠️ Potential issue | 🟡 MinorZero helper balance doesn't verify refund destination.
Similar to the BEP20 assertion issue, checking that the helper has 0 BNB only proves funds left the helper—not that they reached
NORMAL_TIMELOCK. Since BNB is funded at 2× debt, consider asserting that timelock's BNB balance increased by the expected refund amount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 467 - 470, The test "should leave no BNB in the helper" only checks helper.address balance but not that funds reached NORMAL_TIMELOCK; update the test to record the timelock balance before the operation, compute the expected refund amount (the BNB funded to helper, e.g., 2 * debt or the existing refundAmount variable used in the suite), assert helper.balance === 0 as before, then assert ethers.provider.getBalance(timelock.address) increased by the expected refund (use timelock.address and expect(newTimelockBalance).to.equal(oldTimelockBalance.add(expectedRefund))). Ensure you reference helper.address, timelock.address, ethers.provider.getBalance and the same refund/debt variable used elsewhere in the test.
424-437:⚠️ Potential issue | 🟡 MinorWeak assertion doesn't verify refund recipient.
The test checks
postTimelockBalance >= preTimelockBalancewhich only ensures timelock didn't lose tokens. Since the test funds each market with 2×debt, there should be a positive refund to timelock. Asserting a strict increase (>) for funded markets would verify the refund actually reached the timelock.💡 Suggested improvement
it("should return unused BEP20 tokens to timelock", async () => { - // For each token, timelock should have gotten back any excess for (const market of BEP20_MARKETS) { const token = ERC20__factory.connect(market.underlying, ethers.provider); const postTimelockBalance = await token.balanceOf(NORMAL_TIMELOCK); const preTimelockBalance = preTimelockBalances[market.underlying]; - // Timelock balance should be >= pre-balance (got back unused tokens) - expect(postTimelockBalance).to.be.gte( + // Timelock balance should increase (got back unused tokens from 2x funding) + expect(postTimelockBalance).to.be.gt( preTimelockBalance, - `Timelock ${market.name} balance should not decrease`, + `Timelock ${market.name} balance should increase from refunded tokens`, ); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/badDebtHelper.ts` around lines 424 - 437, The current test "should return unused BEP20 tokens to timelock" only asserts postTimelockBalance >= preTimelockBalance, which doesn't verify a refund; update the assertion to require a strict increase for markets that were funded (use BEP20_MARKETS and preTimelockBalances to identify funded markets) by replacing the non-strict gte check with a strict gt check (e.g., expect(postTimelockBalance).to.be.gt(preTimelockBalance)) when verifying ERC20__factory.connect(...).balanceOf(NORMAL_TIMELOCK) so the test ensures timelock actually received the refunded tokens.
🧹 Nitpick comments (1)
contracts/Tokens/VTokens/BadDebtHelper.sol (1)
363-374: Consider adding early return when balance is zero.If
balance == 0anddebt > 0, the function callsrepayBorrowBehalf(borrower, 0)which is a no-op but wastes gas. An early return would optimize this edge case.♻️ Optional gas optimization
uint256 debt = vToken.borrowBalanceStored(borrower); if (debt == 0) return; uint256 balance = underlying.balanceOf(address(this)); + if (balance == 0) return; uint256 repayAmount = debt <= balance ? type(uint256).max : balance;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/Tokens/VTokens/BadDebtHelper.sol` around lines 363 - 374, Add an early return when the contract's underlying balance is zero to avoid calling repayBorrowBehalf with a zero amount; locate the block using vToken.borrowBalanceStored(borrower), underlying.balanceOf(address(this)), and repayBorrowBehalf(borrower, repayAmount) and insert a check like `if (balance == 0) return;` before computing repayAmount and calling vToken.repayBorrowBehalf, keeping the existing error handling and emit BadDebtRepaid logic intact.
🤖 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/badDebtHelper.ts`:
- Around line 317-321: The loop that calls findBalanceSlot(market.underlying)
currently logs and continues when slot === null, which lets later debt
assertions run against markets that were never funded; change this so failures
are explicit: either throw an error when findBalanceSlot returns null to fail
fast (so fix by replacing the console warning+continue with a thrown Error
referencing the market.name and findBalanceSlot), or maintain a skipped list
(e.g., skippedMarkets) when slot === null and then exclude any market in
skippedMarkets from the later debt assertion block; reference findBalanceSlot,
the slot variable, and the debt assertion logic to implement one of these two
behaviors.
- Around line 467-470: The test "should leave no BNB in the helper" only checks
helper.address balance but not that funds reached NORMAL_TIMELOCK; update the
test to record the timelock balance before the operation, compute the expected
refund amount (the BNB funded to helper, e.g., 2 * debt or the existing
refundAmount variable used in the suite), assert helper.balance === 0 as before,
then assert ethers.provider.getBalance(timelock.address) increased by the
expected refund (use timelock.address and
expect(newTimelockBalance).to.equal(oldTimelockBalance.add(expectedRefund))).
Ensure you reference helper.address, timelock.address,
ethers.provider.getBalance and the same refund/debt variable used elsewhere in
the test.
- Around line 424-437: The current test "should return unused BEP20 tokens to
timelock" only asserts postTimelockBalance >= preTimelockBalance, which doesn't
verify a refund; update the assertion to require a strict increase for markets
that were funded (use BEP20_MARKETS and preTimelockBalances to identify funded
markets) by replacing the non-strict gte check with a strict gt check (e.g.,
expect(postTimelockBalance).to.be.gt(preTimelockBalance)) when verifying
ERC20__factory.connect(...).balanceOf(NORMAL_TIMELOCK) so the test ensures
timelock actually received the refunded tokens.
---
Nitpick comments:
In `@contracts/Tokens/VTokens/BadDebtHelper.sol`:
- Around line 363-374: Add an early return when the contract's underlying
balance is zero to avoid calling repayBorrowBehalf with a zero amount; locate
the block using vToken.borrowBalanceStored(borrower),
underlying.balanceOf(address(this)), and repayBorrowBehalf(borrower,
repayAmount) and insert a check like `if (balance == 0) return;` before
computing repayAmount and calling vToken.repayBorrowBehalf, keeping the existing
error handling and emit BadDebtRepaid logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e150114-26e6-441a-b8f7-10153339ea03
📒 Files selected for processing (2)
contracts/Tokens/VTokens/BadDebtHelper.soltests/hardhat/Fork/badDebtHelper.ts
|
lgtm, lets not merge it as it's one-off will keep it WIP as a good ref |
fred-venus
changed the title
fred-venus
changed the title
…change rate The second sweep is now limited to TOTAL_THE_SWEEP - firstSweep, leaving sufficient internalCash in vTHE so the exchange rate stays > 1.
…lock Excess BEP20 and BNB after repayments should go to Treasury, not Normal Timelock. Added fork tests asserting Treasury receives funds and Timelock balance does not increase.
|
2 participants
Summary
BadDebtHelpercontract that atomically repays bad debt across 19 BEP20 tokens + native BNB in the BSC Core PoolChanges
Contract (
BadDebtHelper.sol)_repayTHE(): accepts vTHE admin → sweeps THE → repays 8 borrowers → transfers remaining THE to target receiver → returns admin_repayBEP20(): repays bad debt for 17 BEP20 tokens across 20+ borrowers using liveborrowBalanceStored, returns unused tokens to timelock_repayBNB(): repays 5 BNB borrowers with fixed amounts, returns unused BNB to timelockFork test (
badDebtHelper.ts)Test plan
FORKED_NETWORK=bscmainnet npx hardhat test tests/hardhat/Fork/badDebtHelper.ts— 28 passingSummary by CodeRabbit
New Features
Chores
Tests