Feat: protocol fee split #43

karczuRF · 2022-05-11T14:38:42Z

Summary

Closes #39
The goal is to upgrade FeeReceiver contract to solve two issues:

trying to liquidate in pair with small liquidity
add protocol fee split feature

New features

change DXswapFeeReceiver contract to protect from situation where takeProtocolFee function tries to swap token in pair with too small liquidity
allow to control fee split and external receiver for each pair
supports max one external receiver address
no deadline to claim protocol fee

kowalski · 2022-05-12T10:41:56Z

contracts/DXswapFeeReceiver.sol

+        (uint256 reserveIn, uint256 reserveOut) = token0 < token1 ? (reserve0, reserve1) : (reserve1, reserve0);
+        if (reserveIn == 0 || reserveOut == 0) return false;
+
+        uint256 priceImpact = amount.mul(10000) / reserveIn; // simplified formula


I guess either use SafeMath or not. Let's not mix:)

so either:

amount.mul(10000).div(reserveIn);

or

amount * 10000 / reserveIn

luzzif · 2022-05-17T15:15:29Z

contracts/DXswapFeeReceiver.sol

    address public WETH;
    address public ethReceiver;
    address public fallbackReceiver;
+    uint32 public maxSwapPriceImpact = 100; // uses default 1% as max allowed price impact for takeProtocolFee swap


If this is expressed in basis points it can probably be a uint16.

luzzif · 2022-05-17T15:18:31Z

contracts/DXswapFeeReceiver.sol


    address public owner;
    IDXswapFactory public factory;
    address public WETH;


I can't remember if this is actually intended to be WETH or just the deployment chain's native currency wrapper (WXDAI on GC for example). If the latter applies, I'd suggest a rename to nativeCurrencyWrapper or something like that.

P.S.: deriving from the below logic, it looks like WETH should indeed be called in a more network-neutral way.

I think the framework has been to convert the fees, which start in the form of LP tokens, into the chain's native currency if possible. This has made sense thus far with Ethereum Mainnet, Arbitrum (using ETH as native currency), and Gnosis Chain (using xDai). However, I think generally speaking it's not obvious that the protocol fee should always be converted to native currency. It could depend a lot on which chain. Maybe we should consider making the target to convert the fee to more flexible? But in the context of how DXdao has been using the feeReceiver, then the above comment is correct that the intent is to be the chain's native currency and not always WETH.

Thing is, this contract only really works with the target chain's native asset and its wrapper as is (as you recognize). If we want to make this more flexible I think we have two options:

If we want to keep the current logic unchanged, another contract would be needed, dealing with a standard ERC20 token for example.

Even though this option would alter the logic a little bit (in my opinion in very acceptable or even desirable ways) I'd avoid calling withdraw on the wrapper when executing _takeETHorToken, have the contract send wrapped native currency to the recipient and convert the above variable to a standard ERC20 named for example targetToken or something. This is desirable to simplify the logic, reducing gas consumption and attack surface. As is, for each harvested LP token that is convertible to the native currency (or, actually, its wrapper) we also perform the withdraw. It might be more sensible to offload the withdraw to the recipient for it to be performed only when actually needed.

I think the approach of the second option makes sense. The first implementation deployment of the feeReceiver was on mainnet and the recipient was the bonding curve, which needs to be sent ETH in order for it to work. That fact probably influenced our initial implementation. However, I don't think the new implementation needs to send to the bonding curve, as the bonding curve is now paused and in all likelihood will be sunset.

I agree that the second option makes sense!
Ok, so if we decide not to swap to ETH at all then the _swapTokensForETH function can be removed as well as withdraw + safeTransferETH. Instead the new argument will be added like targetToken. By default feeReceiver will try to swap to targetToken which is ERC20 and if it's not possible send LP to the fallbackReceiver.
Am I right or misunderstood sth?

luzzif · 2022-05-17T15:19:10Z

contracts/DXswapFeeReceiver.sol

-        address _owner, address _factory, address _WETH, address _ethReceiver, address _fallbackReceiver
+        address _owner,
+        address _factory,
+        address _WETH,


If above applies, this should be changed too.

luzzif · 2022-05-17T15:25:51Z

contracts/DXswapFeeReceiver.sol

 import './interfaces/IDXswapFactory.sol';
 import './interfaces/IDXswapPair.sol';
 import './interfaces/IWETH.sol';
 import './libraries/TransferHelper.sol';


I'd honestly upgrade to Solidity v0.8.13 or later if available and leverage both:

The fact that with Sol >=v0.8.0, safe math is not really required anymore since under/overflow checks are automatically performed.

The Yul optimizer is production ready and I'd use it to shave off some gas consumption.

jpkcambridge · 2022-08-18T21:02:59Z

contracts/DXswapFeeReceiver.sol

+        emit TakeProtocolFee(msg.sender, ethReceiver, pairs.length);
+    }
+
+    // called by the owner to set maximum swap price impact allowed for single token-weth swap (within 0-100% range)


Should maybe clarify what number correspond to what percentages

jpkcambridge · 2022-08-18T21:12:52Z

contracts/DXswapFeeReceiver.sol

+        (uint256 reserveIn, uint256 reserveOut) = token0 < token1 ? (reserve0, reserve1) : (reserve1, reserve0);
+        if (reserveIn == 0 || reserveOut == 0) return false;
+
+        uint256 priceImpact = amount.mul(10000) / reserveIn; // simplified formula


I see this is intentionally a simplified formula. Do we think it's good enough?

I've done quite a lot of calculations (if you're interested in details I can share the sheet) and imo it's good enough just as an extra protection step. Ofc it is possible to implement more complex formula, but that could cost more. I'll check it out and compare gas usage.

I'd like to see more if possible.

luzzif · 2022-08-29T21:49:59Z

contracts/DXswapFeeReceiver.sol


    address public owner;
    IDXswapFactory public factory;
    address public WETH;


Thing is, this contract only really works with the target chain's native asset and its wrapper as is (as you recognize). If we want to make this more flexible I think we have two options:

If we want to keep the current logic unchanged, another contract would be needed, dealing with a standard ERC20 token for example.

Even though this option would alter the logic a little bit (in my opinion in very acceptable or even desirable ways) I'd avoid calling withdraw on the wrapper when executing _takeETHorToken, have the contract send wrapped native currency to the recipient and convert the above variable to a standard ERC20 named for example targetToken or something. This is desirable to simplify the logic, reducing gas consumption and attack surface. As is, for each harvested LP token that is convertible to the native currency (or, actually, its wrapper) we also perform the withdraw. It might be more sensible to offload the withdraw to the recipient for it to be performed only when actually needed.

luzzif · 2022-08-29T21:53:59Z

contracts/DXswapFeeReceiver.sol

+
+    mapping(address => ExternalFeeReceiver) public externalFeeReceivers;
+
+    event TakeProtocolFee(address indexed sender, address indexed to, uint256 NumberOfPairs);


Change NumberOfPairs to camel case.

luzzif · 2022-08-29T21:56:26Z

contracts/DXswapFeeReceiver.sol

    function() external payable {}

+    // called by the owner to set the new owner
    function transferOwnership(address newOwner) external {


I'm actually becoming a fan of the acceptOwnership method of doing ownership transfers. In case ownership of a contract is delicate, better to implement this pattern with increased security.

luzzif · 2022-08-29T22:00:04Z

contracts/DXswapFeeReceiver.sol

-    function isContract(address addr) internal returns (bool) {
-        uint size;
-        assembly { size := extcodesize(addr) }
+    function isContract(address addr) internal view returns (bool) {


This must be replaced by a call to the factory. Increases gas consumption a little bit, but since it's only used to check if a pair exists, in the process we also have guarantees about the validity of a pair (and we avoid surprises like the hack we've had).

luzzif · 2022-08-29T22:02:47Z

contracts/DXswapFeeReceiver.sol

+        (uint256 reserveIn, uint256 reserveOut) = token0 < token1 ? (reserve0, reserve1) : (reserve1, reserve0);
+        if (reserveIn == 0 || reserveOut == 0) return false;
+
+        uint256 priceImpact = amount.mul(10000) / reserveIn; // simplified formula


I'd like to see more if possible.

luzzif · 2022-08-29T22:12:14Z

contracts/DXswapFeeReceiver.sol

+    // called by the owner to set maximum swap price impact allowed for single token-weth swap (within 0-100% range)
+    function setMaxSwapPriceImpact(uint32 _maxSwapPriceImpact) external {
+        require(msg.sender == owner, 'DXswapFeeReceiver: CALLER_NOT_OWNER');
+        require(_maxSwapPriceImpact > 0 && _maxSwapPriceImpact < 10000, 'DXswapFeeReceiver: FORBIDDEN_PRICE_IMPACT');


The first check _maxSwapPriceImpact > 0 is not needed since we're dealing with unsigned integers.

Indeed it's not needed for unsigned integers, but this check is done to not to set priceImpact to zero.

luzzif · 2022-08-29T22:14:13Z

contracts/DXswapFeeReceiver.sol

+    // called by the owner to set fee percentage to external receiver
+    function setFeePercentageToExternalReceiver(address _pair, uint32 _feePercentageToExternalReceiver) external {
+        require(msg.sender == owner, 'DXswapFeeReceiver: CALLER_NOT_OWNER');
+        IDXswapPair swapPair = IDXswapPair(_pair);


Please let's not assume the input is a Swapr pair, not even if only the owner can perform the call here.

luzzif · 2022-08-29T22:14:48Z

contracts/DXswapFeeReceiver.sol

+        uint256 feeReceiverBalance = swapPair.balanceOf(address(this));
+        if (feeReceiverBalance > 0) {
+            // withdraw accumulated fees before updating the split percentage
+            address token0 = swapPair.token0();


This common logic could probably be extracted in a function to avoid duplicated code.

luzzif · 2022-08-29T22:15:58Z

contracts/DXswapFeeReceiver.sol

+            if (amount1 > 0) _takeTokenOrETH(address(swapPair), token1, amount1);
+            emit TakeProtocolFee(msg.sender, ethReceiver, 1);
+        }
+        require(swapPair.balanceOf(address(this)) == 0, 'DXswapFeeReceiver: TOKENS_NOT_BURNED');


This might fail. When performing burn some fee tokens might be minted depending on the pair's activity.

luzzif · 2022-08-29T22:16:43Z

contracts/DXswapFeeReceiver.sol

+
+        // fee percentage check
+        require(
+            _feePercentageToExternalReceiver >= 0 && _feePercentageToExternalReceiver <= 5000,


_feePercentageToExternalReceiver >= 0 can be removed, dealing with unsigned integers.

…by calling factory

karczuRF added 3 commits May 11, 2022 16:07

feat: add protocol fee split feature

34f449d

test: feeReceiver split fee

76a95eb

test: increase gas limit

48a0c85

kowalski reviewed May 12, 2022

View reviewed changes

luzzif suggested changes May 17, 2022

View reviewed changes

karczuRF added 3 commits June 28, 2022 15:41

refactor: migrate to hardhat

08e3a5c

undo feeSplitter changes and fix tests

275ac5b

feat: add FeeSplitter contract & test

1d05cbb

jpkcambridge reviewed Aug 18, 2022

View reviewed changes

luzzif suggested changes Aug 29, 2022

View reviewed changes

karczuRF added 7 commits August 30, 2022 16:49

refactor after cr: rename & remove reduntant checks

ad1ca78

refactor: remove isContract & pairFor internal functions and replace …

8dff165

…by calling factory

refactor: upgrade sol & price impact formula & lint

677acb5

Merge branch 'feat/DXswapFeeSplitter' into feat/protocol-fee-split

3ae81fb

fix bugs after master merge

721aa22

Merge branch 'master' into feat/protocol-fee-split

5572f39

fix bugs after merge

dadbb47

karczuRF marked this pull request as ready for review December 16, 2022 13:40


		mapping(address => ExternalFeeReceiver) public externalFeeReceivers;

		event TakeProtocolFee(address indexed sender, address indexed to, uint256 NumberOfPairs);

Feat: protocol fee split #43

Are you sure you want to change the base?

Feat: protocol fee split #43

Uh oh!

Conversation

karczuRF commented May 11, 2022

Summary

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!

jpkcambridge Aug 18, 2022 • 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!

luzzif Aug 29, 2022 • 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!

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!

luzzif Aug 29, 2022 • 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!

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!

5 participants

jpkcambridge Aug 18, 2022 •

edited

Loading

luzzif Aug 29, 2022 •

edited

Loading

luzzif Aug 29, 2022 •

edited

Loading