Skip to content

Commit 554d431

Browse files
tjcloatjcloa
committed
address review comments
1 parent 6913b71 commit 554d431

3 files changed

Lines changed: 174 additions & 18 deletions

File tree

‎contracts/governance/FeeSharingCollector/FeeSharingCollector.sol‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,6 @@ contract FeeSharingCollector is
252252
* @notice Add checkpoint with accumulated amount by function invocation.
253253
* @dev If token is in protocol withhold list, accumulates fees for protocol (no checkpoint for stakers).
254254
* @dev If token is not in withhold list, creates checkpoint for stakers (100% distribution).
255-
* @dev Does not create zero-value checkpoints to save gas.
256255
* @param _token Address of the token.
257256
* @param _amount Amount of tokens to process.
258257
* */
@@ -1056,6 +1055,7 @@ contract FeeSharingCollector is
10561055
* @dev Only callable by owner.
10571056
* @dev Fees from tokens in this list are withheld 100% by protocol (no staker distribution).
10581057
* @dev If there are unprocessed fees accumulated before adding to list, they are moved to protocolWithheldFees.
1058+
* @dev Resets lastFeeWithdrawalTime to avoid stale interval behavior if token is later removed from list.
10591059
* @param token The token address to add.
10601060
*/
10611061
function addProtocolWithholdToken(address token) external onlyOwner {
@@ -1077,12 +1077,15 @@ contract FeeSharingCollector is
10771077
unprocessedAmount[token] = 0;
10781078
}
10791079

1080+
/// @notice Reset withdrawal interval baseline while token is in withhold mode.
1081+
lastFeeWithdrawalTime[token] = 0;
1082+
10801083
protocolWithheldTokensList.add(token);
10811084
emit TokenAddedToProtocolWithholdList(msg.sender, token);
10821085
}
10831086

10841087
/**
1085-
* @notice Remove a token from the protocol withhold list.
1088+
* @notice Remove a token from the protocol withhold list. Accumulated protocol fees can still be withdrawn by owner, but new fees will be distributed to stakers.
10861089
* @dev Only callable by owner.
10871090
* @dev Fees from tokens removed from this list will be distributed to stakers.
10881091
* @param token The token address to remove.

‎deployment/deploy/1020-deploy-FeeSharingCollector.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ const func = async function (hre) {
4444
const withholdTokens = [
4545
{ name: "RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT", address: rbtcDummyAddress },
4646
{ name: "ZUSD", address: zusdToken },
47-
{ name: "WRBTC", address: wrbtcToken },
4847
];
4948

5049
for (const token of withholdTokens) {

‎tests/FeeSharingCollectorTokenWithholdingTest.js‎

Lines changed: 169 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const ILoanTokenLogicProxy = artifacts.require("ILoanTokenLogicProxy");
2727
const ILoanTokenModules = artifacts.require("ILoanTokenModules");
2828
const FeeSharingCollector = artifacts.require("FeeSharingCollector");
2929
const FeeSharingCollectorProxy = artifacts.require("FeeSharingCollectorProxy");
30+
const LiquidityPoolV1Converter = artifacts.require("LiquidityPoolV1ConverterMockup");
3031

3132
const mutexUtils = require("../deployment/helpers/reentrancy/utils");
3233

@@ -321,10 +322,6 @@ contract("FeeSharingCollector - Token-Specific Withholding", (accounts) => {
321322
"0",
322323
"Should not create checkpoint without transfers"
323324
);
324-
325-
// The actual zero-value protection is in _addCheckpoint:
326-
// if (amount > 0) { _writeTokenCheckpoint(_token, amount); }
327-
// This prevents creating a checkpoint when accumulated amount is 0
328325
});
329326

330327
it("should handle RBTC withholding", async () => {
@@ -454,6 +451,173 @@ contract("FeeSharingCollector - Token-Specific Withholding", (accounts) => {
454451
totalCheckpoints = await feeSharingCollector.totalTokenCheckpoints(SOVToken.address);
455452
assert.equal(totalCheckpoints.toString(), "1", "Should not create new checkpoint");
456453
});
454+
455+
it("should reset fee interval baseline when token is added to withhold list", async () => {
456+
const firstAmount = new BN(1);
457+
const secondAmount = new BN(2);
458+
459+
// Initial transfer creates first checkpoint and sets lastFeeWithdrawalTime.
460+
await SOVToken.approve(feeSharingCollector.address, firstAmount, { from: root });
461+
await feeSharingCollector.transferTokens(SOVToken.address, firstAmount, {
462+
from: root,
463+
});
464+
465+
let totalCheckpoints = await feeSharingCollector.totalTokenCheckpoints(
466+
SOVToken.address
467+
);
468+
assert.equal(totalCheckpoints.toString(), "1", "Initial checkpoint should be created");
469+
470+
await feeSharingCollector.addProtocolWithholdToken(SOVToken.address, { from: root });
471+
472+
const lastFeeWithdrawalTime = await feeSharingCollector.lastFeeWithdrawalTime(
473+
SOVToken.address
474+
);
475+
assert.equal(
476+
lastFeeWithdrawalTime.toString(),
477+
"0",
478+
"lastFeeWithdrawalTime should be reset while token is withheld"
479+
);
480+
481+
// Remove from withhold list before interval passes and transfer again.
482+
await feeSharingCollector.removeProtocolWithholdToken(SOVToken.address, {
483+
from: root,
484+
});
485+
await SOVToken.approve(feeSharingCollector.address, secondAmount, { from: root });
486+
await feeSharingCollector.transferTokens(SOVToken.address, secondAmount, {
487+
from: root,
488+
});
489+
490+
// Because the baseline was reset, transfer creates checkpoint immediately.
491+
totalCheckpoints = await feeSharingCollector.totalTokenCheckpoints(SOVToken.address);
492+
assert.equal(
493+
totalCheckpoints.toString(),
494+
"2",
495+
"Should create checkpoint immediately after token is removed from withhold list"
496+
);
497+
498+
const unprocessedAfter = await feeSharingCollector.unprocessedAmount(SOVToken.address);
499+
assert.equal(unprocessedAfter.toString(), "0", "No unprocessed amount should remain");
500+
});
501+
502+
it("should withhold RBTC from withdrawFees() when RBTC dummy token is withheld", async () => {
503+
const feeAmount = new BN(wei("1", "ether"));
504+
505+
await feeSharingCollector.addProtocolWithholdToken(RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT, {
506+
from: root,
507+
});
508+
509+
// Seed protocol fee accounting and token balance so protocol.withdrawFees can transfer WRBTC.
510+
await sovryn.setLendingFeeTokensHeld(WRBTC.address, feeAmount);
511+
await WRBTC.deposit({ from: root, value: feeAmount });
512+
await WRBTC.transfer(sovryn.address, feeAmount, { from: root });
513+
514+
const tx = await feeSharingCollector.withdrawFees([WRBTC.address], { from: root });
515+
516+
expectEvent(tx, "FeeWithdrawnInRBTC", {
517+
sender: root,
518+
amount: feeAmount,
519+
});
520+
expectEvent(tx, "ProtocolRevenueAccumulated", {
521+
token: RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT,
522+
amount: feeAmount,
523+
});
524+
525+
const withheldFees = await feeSharingCollector.getProtocolWithheldFees(
526+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
527+
);
528+
assert.equal(
529+
withheldFees.toString(),
530+
feeAmount.toString(),
531+
"RBTC should be accumulated as withheld protocol fees"
532+
);
533+
534+
const totalCheckpoints = await feeSharingCollector.totalTokenCheckpoints(
535+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
536+
);
537+
assert.equal(
538+
totalCheckpoints.toString(),
539+
"0",
540+
"No staker checkpoint should be created"
541+
);
542+
543+
const unprocessed = await feeSharingCollector.unprocessedAmount(
544+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
545+
);
546+
assert.equal(
547+
unprocessed.toString(),
548+
"0",
549+
"No unprocessed amount should be accumulated"
550+
);
551+
});
552+
553+
it("should withhold RBTC from withdrawFeesAMM() when RBTC dummy token is withheld", async () => {
554+
const feeAmount = new BN(wei("1", "ether"));
555+
const liquidityPoolV1Converter = await LiquidityPoolV1Converter.new(
556+
SOVToken.address,
557+
SUSD.address
558+
);
559+
560+
await liquidityPoolV1Converter.setTotalFeeMockupValue(feeAmount.toString());
561+
await liquidityPoolV1Converter.setFeesController(feeSharingCollector.address);
562+
await liquidityPoolV1Converter.setWrbtcToken(WRBTC.address);
563+
await WRBTC.deposit({ from: root, value: feeAmount });
564+
await WRBTC.transfer(liquidityPoolV1Converter.address, feeAmount, { from: root });
565+
566+
await feeSharingCollector.addWhitelistedConverterAddress(
567+
liquidityPoolV1Converter.address,
568+
{
569+
from: root,
570+
}
571+
);
572+
await feeSharingCollector.addProtocolWithholdToken(RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT, {
573+
from: root,
574+
});
575+
576+
const tx = await feeSharingCollector.withdrawFeesAMM(
577+
[liquidityPoolV1Converter.address],
578+
{
579+
from: root,
580+
}
581+
);
582+
583+
expectEvent(tx, "FeeAMMWithdrawn", {
584+
sender: root,
585+
converter: liquidityPoolV1Converter.address,
586+
amount: feeAmount,
587+
});
588+
589+
expectEvent(tx, "ProtocolRevenueAccumulated", {
590+
token: RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT,
591+
amount: feeAmount,
592+
});
593+
594+
const withheldFees = await feeSharingCollector.getProtocolWithheldFees(
595+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
596+
);
597+
assert.equal(
598+
withheldFees.toString(),
599+
feeAmount.toString(),
600+
"AMM RBTC should be accumulated as withheld protocol fees"
601+
);
602+
603+
const totalCheckpoints = await feeSharingCollector.totalTokenCheckpoints(
604+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
605+
);
606+
assert.equal(
607+
totalCheckpoints.toString(),
608+
"0",
609+
"No staker checkpoint should be created"
610+
);
611+
612+
const unprocessed = await feeSharingCollector.unprocessedAmount(
613+
RBTC_DUMMY_ADDRESS_FOR_CHECKPOINT
614+
);
615+
assert.equal(
616+
unprocessed.toString(),
617+
"0",
618+
"No unprocessed amount should be accumulated"
619+
);
620+
});
457621
});
458622

459623
describe("Withdraw Protocol Withheld Fees", () => {
@@ -569,17 +733,7 @@ contract("FeeSharingCollector - Token-Specific Withholding", (accounts) => {
569733

570734
describe("Gas Optimization - Zero-Value Checkpoint Prevention", () => {
571735
it("verifies _addCheckpoint logic prevents zero-value checkpoints", async () => {
572-
// The zero-value checkpoint prevention is implemented in _addCheckpoint:
573-
//
574-
// if (protocolWithheldTokensList.contains(_token)) {
575-
// protocolWithheldFees[_token] += amount; // No checkpoint
576-
// } else {
577-
// if (amount > 0) { // Only create if amount > 0
578-
// _writeTokenCheckpoint(_token, amount);
579-
// }
580-
// }
581-
//
582-
// This prevents creating checkpoints when unprocessedAmount + _amount = 0
736+
//The actual protection comes from the fact that transferTokens and transferRBTC require _amount > 0
583737

584738
// Verify initial state has no checkpoints
585739
await increaseTime(FEE_WITHDRAWAL_INTERVAL + 1);

0 commit comments

Comments
 (0)