SOV-4106 Feat: Fee sharing collector with direct transfer #2

cwsnt · 2024-06-14T23:21:23Z

No description provided.

koirikivi · 2024-06-24T14:31:41Z

contracts/mixins/ProtocolAccount.sol

+            */
            bytes32 payoutKey = keccak256(abi.encode(recv, token));
            userBals_[payoutKey].surplusCollateral_ += collected;
+            require(userBals_[payoutKey].surplusCollateral_ <= type(uint96).max, "Value exceeds uint96 range");


Not really related to this change, but uint96 can theoretically overflow with something like POWA (where the total potential supply is 1 000 000 000 000 tokens with 18 decimals, or 1e30. uint96 max value is only around ~7.9e28)

good catch
this is the FeeSharingCollector contract's limit.
on the one hand - yes, it is technically possible to collect 1B, but in fact the fees are much less, practical max we had is 0.5%, so it is reduced down to 5e27 and only in case when the total supply is swapped which i doubt is practical.
i think we should restrict tokens with more than 18 deciamls and total supply <= 1B (trillion) for pools creation.
in case we ever reach the limit, we will upgrade (replace) the fee sharing collector.

koirikivi

One comment about uint96, not related to this change really.

tjcloa

see inline comments

tjcloa · 2024-06-26T16:02:39Z

contracts/callpaths/ColdPath.sol

        (, address treasury) = abi.decode(cmd, (uint8, address));

        require(treasury != address(0) && treasury.code.length != 0, "Treasury invalid");
        treasury_ = treasury;


to keep it universal i suggest to restore the original code

treasuryStartTime_ = uint64(block.timestamp + 7 days);

but replace 7 days with a constant TREASURY_START_TIME_OFFSET to have this option if needed on other chains.
The BOB and default implementation should have it set to 0.

tjcloa · 2024-06-26T16:11:33Z

contracts/callpaths/LongPath.sol

 import '../libraries/Encoding.sol';
 import '../libraries/TokenFlow.sol';
 import '../libraries/PriceGrid.sol';
+import '../libraries/ProtocolCmd.sol';


why do you need it here?

tjcloa · 2024-06-26T16:12:58Z

contracts/interfaces/ISdexQuery.sol

+
+import "../libraries/PoolSpecs.sol";
+
+interface ISdexQuery {


this interface is not used anywhere

tjcloa · 2024-06-26T16:15:02Z

contracts/libraries/README.md

@@ -0,0 +1,3 @@
+# SdexConvertLib


there is no SdexConvertLib
do you want to keep this README.md?

tjcloa · 2024-06-26T16:16:47Z

contracts/mixins/ProtocolAccount.sol

     * @param recv - The receiver of the protocol fees.
-     * @param token - The token address of the quote token. */
+     * @param token - The token address of the quote token.
+     * @return withdrawn tokens  */


in doesn't return anything

tjcloa · 2024-06-26T18:22:20Z

test/TestPool.govern.ts

+    it("successfully collect treasury without time delay", async() => {
      await test.testRevisePool(feeRate, 128, 1) // Turn on protocol fee
-      await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy.address), true)
+      await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy2.address), true)


why is policy replaced with policy2 here?

tjcloa · 2024-06-26T19:21:50Z

contracts/mixins/ProtocolAccount.sol

+            /**
+            * directly deposit token to fee protocol collector
+            */
            bytes32 payoutKey = keccak256(abi.encode(recv, token));


add a constant PROTOCOL_FEES_RECEIVER_HASH - hash of 'PROTOCOL_FEES_RECEIVER' + some nonce - can be a hardcoded hash and use instead of recv because should we replace the FeeSharingCollector, the fees collected should be paid out to the new FSC, otherwise this portion will stuck or we would need to call from the previous FSC which is tricky - need not to forget to do it. once replaced - we should only transfer all the fees to he new FSC - one time operation.

Suggested change

bytes32 payoutKey = keccak256(abi.encode(recv, token));

bytes32 payoutKey = keccak256(abi.encode(PROTOCOL_FEES_RECEIVER_HASH, token));

tjcloa · 2024-06-26T19:24:23Z

contracts/mixins/ProtocolAccount.sol

-     * @param token - The token address of the quote token. */
+     * @param token - The token address of the quote token.
+     * @return withdrawn tokens  */
    function disburseProtocolFees (address recv, address token) internal {


we don't need a receiver (recv) here. also see comments below for clarity.
tokens should be an array

Suggested change

function disburseProtocolFees (address recv, address token) internal {

function disburseProtocolFees (address[] tokens) internal {

tjcloa · 2024-06-26T19:30:38Z

contracts/mixins/ProtocolAccount.sol


+import '../interfaces/IFeeProtocolCollector.sol';
+
 /* @title Protocol Account Mixin


the ProtocolAccount contract is inherited in WarmPath, but seems not to be used there.
pls double check and remove the inheritance.
the contract doesn't have it own storage, so should be safe.

tjcloa · 2024-06-26T19:46:17Z

contracts/mixins/ProtocolAccount.sol

            bytes32 payoutKey = keccak256(abi.encode(recv, token));
            userBals_[payoutKey].surplusCollateral_ += collected;
+            require(userBals_[payoutKey].surplusCollateral_ <= type(uint96).max, "Value exceeds uint96 range");
+            IFeeProtocolCollector(treasury_).transferTokens(token, uint96(userBals_[payoutKey].surplusCollateral_));


FeeSharingCollector requires tokens approval for ERC20 tokens which is missing here.
1.2 For the native token the function should be called with value payload.

pls add tests - would have helped to discover it.

cwsnt added 4 commits May 24, 2024 13:26

feat: support fee sharing collector

ce99bf0

init: convertion lib dex

9f4da78

add readme for on going lib

187c965

init: collect protocol fee with direct transfer

97b3b40

cwsnt requested a review from tjcloa June 14, 2024 23:21

cwsnt changed the title ~~Feat: Fee sharing collector with direct transfer~~ SOV-4106 Feat: Fee sharing collector with direct transfer Jun 24, 2024

koirikivi reviewed Jun 24, 2024

View reviewed changes

koirikivi approved these changes Jun 24, 2024

View reviewed changes

tjcloa requested changes Jun 26, 2024

View reviewed changes

1/ address comment

aa9b3d4

	bytes32 payoutKey = keccak256(abi.encode(recv, token));
	bytes32 payoutKey = keccak256(abi.encode(PROTOCOL_FEES_RECEIVER_HASH, token));

	function disburseProtocolFees (address recv, address token) internal {
	function disburseProtocolFees (address[] tokens) internal {


		import '../interfaces/IFeeProtocolCollector.sol';

		/* @title Protocol Account Mixin

SOV-4106 Feat: Fee sharing collector with direct transfer #2

Are you sure you want to change the base?

SOV-4106 Feat: Fee sharing collector with direct transfer #2

Uh oh!

Conversation

cwsnt commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjcloa Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koirikivi left a comment

Choose a reason for hiding this comment

Uh oh!

tjcloa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tjcloa Jun 26, 2024 •

edited

Loading