Skip to content

Feat: protocol fee split #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
karczuRF wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Feat: protocol fee split #43

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
34f449d
feat: add protocol fee split feature
karczuRF May 11, 2022
76a95eb
test: feeReceiver split fee
karczuRF May 11, 2022
48a0c85
test: increase gas limit
karczuRF May 11, 2022
08e3a5c
refactor: migrate to hardhat
karczuRF Jun 28, 2022
275ac5b
undo feeSplitter changes and fix tests
karczuRF Jul 5, 2022
1d05cbb
feat: add FeeSplitter contract & test
karczuRF Jul 5, 2022
ad1ca78
refactor after cr: rename & remove reduntant checks
karczuRF Aug 30, 2022
8dff165
refactor: remove isContract & pairFor internal functions and replace …
karczuRF Sep 1, 2022
677acb5
refactor: upgrade sol & price impact formula & lint
karczuRF Sep 5, 2022
3ae81fb
Merge branch 'feat/DXswapFeeSplitter' into feat/protocol-fee-split
karczuRF Dec 15, 2022
721aa22
fix bugs after master merge
karczuRF Dec 15, 2022
5572f39
Merge branch 'master' into feat/protocol-fee-split
karczuRF Dec 15, 2022
dadbb47
fix bugs after merge
karczuRF Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
228 changes: 166 additions & 62 deletions contracts/DXswapFeeReceiver.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,122 +6,226 @@ import './interfaces/IWETH.sol';
import './libraries/TransferHelper.sol';
Copy link
Copy Markdown

@luzzif luzzif May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The fact that with Sol >=v0.8.0, safe math is not really required anymore since under/overflow checks are automatically performed.
  2. The Yul optimizer is production ready and I'd use it to shave off some gas consumption.

import './libraries/SafeMath.sol';


contract DXswapFeeReceiver {
using SafeMath for uint;
using SafeMath for uint256;

address public owner;
IDXswapFactory public factory;
address public WETH;
Copy link
Copy Markdown

@luzzif luzzif May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@jpkcambridge jpkcambridge Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If we want to keep the current logic unchanged, another contract would be needed, dealing with a standard ERC20 token for example.
  2. 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.

Copy link
Copy Markdown
Member

@jpkcambridge jpkcambridge Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@karczuRF karczuRF Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

@luzzif luzzif May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// if needed set address of external project which can get % of total earned protocol fee
// % of total protocol fee to external project (100 means 1%) is within the range <0, 50>
struct ExternalFeeReceiver {
address externalReceiver;
uint32 feePercentage;
}

mapping(address => ExternalFeeReceiver) public externalFeeReceivers;

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

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change NumberOfPairs to camel case.


constructor(
address _owner, address _factory, address _WETH, address _ethReceiver, address _fallbackReceiver
address _owner,
address _factory,
address _WETH,
Copy link
Copy Markdown

@luzzif luzzif May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If above applies, this should be changed too.

address _ethReceiver,
address _fallbackReceiver
) public {
owner = _owner;
factory = IDXswapFactory(_factory);
WETH = _WETH;
ethReceiver = _ethReceiver;
fallbackReceiver = _fallbackReceiver;
}

function() external payable {}

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

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

require(msg.sender == owner, 'DXswapFeeReceiver: FORBIDDEN');
owner = newOwner;
}


// called by the owner to change receivers addresses
function changeReceivers(address _ethReceiver, address _fallbackReceiver) external {
require(msg.sender == owner, 'DXswapFeeReceiver: FORBIDDEN');
ethReceiver = _ethReceiver;
fallbackReceiver = _fallbackReceiver;
}

// Returns sorted token addresses, used to handle return values from pairs sorted in this order
function sortTokens(address tokenA, address tokenB) internal pure returns (address token0, address token1) {
require(tokenA != tokenB, 'DXswapFeeReceiver: IDENTICAL_ADDRESSES');
(token0, token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
require(token0 != address(0), 'DXswapFeeReceiver: ZERO_ADDRESS');
}

// Helper function to know if an address is a contract, extcodesize returns the size of the code of a smart
// contract in a specific address
function isContract(address addr) internal returns (bool) {
uint size;
assembly { size := extcodesize(addr) }
function isContract(address addr) internal view returns (bool) {
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

uint256 size;
assembly {
size := extcodesize(addr)
}
return size > 0;
}

// Calculates the CREATE2 address for a pair without making any external calls
// Taken from DXswapLibrary, removed the factory parameter
function pairFor(address tokenA, address tokenB) internal view returns (address pair) {
(address token0, address token1) = sortTokens(tokenA, tokenB);
pair = address(uint(keccak256(abi.encodePacked(
hex'ff',
factory,
keccak256(abi.encodePacked(token0, token1)),
hex'd306a548755b9295ee49cc729e13ca4a45e00199bbd890fa146da43a50571776' // init code hash
))));
pair = address(
uint256(
keccak256(
abi.encodePacked(
hex'ff',
factory,
keccak256(abi.encodePacked(token0, token1)),
hex'd306a548755b9295ee49cc729e13ca4a45e00199bbd890fa146da43a50571776' // init code hash
)
)
)
);
}

// Done with code form DXswapRouter and DXswapLibrary, removed the deadline argument
function _swapTokensForETH(uint amountIn, address fromToken)
internal
{
function _swapTokensForETH(uint256 amountIn, address fromToken) internal returns (uint256 amountOut) {
IDXswapPair pairToUse = IDXswapPair(pairFor(fromToken, WETH));

(uint reserve0, uint reserve1,) = pairToUse.getReserves();
(uint reserveIn, uint reserveOut) = fromToken < WETH ? (reserve0, reserve1) : (reserve1, reserve0);

require(reserveIn > 0 && reserveOut > 0, 'DXswapFeeReceiver: INSUFFICIENT_LIQUIDITY');
uint amountInWithFee = amountIn.mul(uint(10000).sub(pairToUse.swapFee()));
uint numerator = amountInWithFee.mul(reserveOut);
uint denominator = reserveIn.mul(10000).add(amountInWithFee);
uint amountOut = numerator / denominator;

TransferHelper.safeTransfer(
fromToken, address(pairToUse), amountIn
);

(uint amount0Out, uint amount1Out) = fromToken < WETH ? (uint(0), amountOut) : (amountOut, uint(0));

pairToUse.swap(
amount0Out, amount1Out, address(this), new bytes(0)
);

IWETH(WETH).withdraw(amountOut);
TransferHelper.safeTransferETH(ethReceiver, amountOut);

(uint256 reserve0, uint256 reserve1, ) = pairToUse.getReserves();
(uint256 reserveIn, uint256 reserveOut) = fromToken < WETH ? (reserve0, reserve1) : (reserve1, reserve0);

require(reserveIn > 0 && reserveOut > 0, 'DXswapFeeReceiver: INSUFFICIENT_LIQUIDITY'); // should never happen since pool was checked before
uint256 amountInWithFee = amountIn.mul(uint256(10000).sub(pairToUse.swapFee()));
uint256 numerator = amountInWithFee.mul(reserveOut);
uint256 denominator = reserveIn.mul(10000).add(amountInWithFee);
amountOut = numerator / denominator;

TransferHelper.safeTransfer(fromToken, address(pairToUse), amountIn);

(uint256 amount0Out, uint256 amount1Out) = fromToken < WETH ? (uint256(0), amountOut) : (amountOut, uint256(0));

pairToUse.swap(amount0Out, amount1Out, address(this), new bytes(0));
}

// Transfer to the owner address the token converted into ETH if possible, if not just transfer the token.
function _takeETHorToken(address token, uint amount) internal {
if (token == WETH) {
// If it is WETH, transfer directly to ETH receiver
IWETH(WETH).withdraw(amount);
TransferHelper.safeTransferETH(ethReceiver, amount);
} else if (isContract(pairFor(token, WETH))) {
// If it is not WETH and there is a direct path to WETH, swap and transfer WETH to ETH receiver
_swapTokensForETH(amount, token);
} else {
// If it is not WETH and there is not a direct path to WETH, transfer tokens directly to fallback receiver
TransferHelper.safeTransfer(token, fallbackReceiver, amount);
}
// Helper function to know if token-WETH pool exists and has enough liquidity
function _isSwapPossible(
address token0,
address token1,
uint256 amount
) internal view returns (bool) {
address pair = pairFor(token0, token1);
if (!isContract(pair)) return false;

(uint256 reserve0, uint256 reserve1, ) = IDXswapPair(pair).getReserves();
(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
Copy link
Copy Markdown
Collaborator

@kowalski kowalski May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

so either:

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

or

amount * 10000 / reserveIn

Copy link
Copy Markdown
Member

@jpkcambridge jpkcambridge Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@karczuRF karczuRF Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see more if possible.

if (priceImpact > maxSwapPriceImpact) return false;

return true;
}

// Checks if LP has an extra external address which participates in the distrubution of protocol fee
// External Receiver address has to be defined and fee % > 0 to transfer tokens
function _splitAndTransferFee(
address pair,
address token,
uint256 amount
) internal {
address _externalFeeReceiver = externalFeeReceivers[pair].externalReceiver;
uint32 _percentFeeToExternalReceiver = externalFeeReceivers[pair].feePercentage;
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case _externalFeeReceiver is the 0 address _percentFeeToExternalReceiver will also be 0 by default. So in some cases we can avoid an extra SLOAD here. Also, I'd suggest doing some experiments on loading the struct in memory like this:

ExternalFeeReceiver memory _externalFeeReceiver = externalFeeReceivers[pair];

and see if using this helps lowering gas consumption.


if (_percentFeeToExternalReceiver > 0 && _externalFeeReceiver != address(0)) {
uint256 feeToExternalReceiver = amount.mul(_percentFeeToExternalReceiver) / 10000;
uint256 feeToAvatarDAO = amount.sub(feeToExternalReceiver);
if (token == WETH) {
IWETH(WETH).withdraw(amount);
TransferHelper.safeTransferETH(_externalFeeReceiver, feeToExternalReceiver);
TransferHelper.safeTransferETH(ethReceiver, feeToAvatarDAO);
} else {
TransferHelper.safeTransfer(token, _externalFeeReceiver, feeToExternalReceiver);
TransferHelper.safeTransfer(token, fallbackReceiver, feeToAvatarDAO);
}
} else {
if (token == WETH) {
IWETH(WETH).withdraw(amount);
TransferHelper.safeTransferETH(ethReceiver, amount);
} else {
TransferHelper.safeTransfer(token, fallbackReceiver, amount);
}
}
}

// Convert tokens into ETH if possible, if not just transfer the token
function _takeTokenOrETH(
address pair,
address token,
uint256 amount
) internal {
if (token != WETH && _isSwapPossible(token, WETH, amount)) {
// If it is not WETH and there is a direct path to WETH, swap tokens
uint256 amountOut = _swapTokensForETH(amount, token);
_splitAndTransferFee(pair, WETH, amountOut);
} else {
// If it is WETH or there is not a direct path from token to WETH, transfer tokens
_splitAndTransferFee(pair, token, amount);
}
}

// Take what was charged as protocol fee from the DXswap pair liquidity
function takeProtocolFee(IDXswapPair[] calldata pairs) external {
for (uint i = 0; i < pairs.length; i++) {
for (uint256 i = 0; i < pairs.length; i++) {
address token0 = pairs[i].token0();
address token1 = pairs[i].token1();
pairs[i].transfer(address(pairs[i]), pairs[i].balanceOf(address(this)));
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we need to perform some validation on the pairs elements here. Having external calls on unverified addresses makes me feel quite uneasy.

(uint amount0, uint amount1) = pairs[i].burn(address(this));
if (amount0 > 0)
_takeETHorToken(token0, amount0);
if (amount1 > 0)
_takeETHorToken(token1, amount1);
(uint256 amount0, uint256 amount1) = pairs[i].burn(address(this));
if (amount0 > 0) _takeTokenOrETH(address(pairs[i]), token0, amount0);
if (amount1 > 0) _takeTokenOrETH(address(pairs[i]), token1, amount1);
}
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)
Copy link
Copy Markdown
Member

@jpkcambridge jpkcambridge Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should maybe clarify what number correspond to what percentages

function setMaxSwapPriceImpact(uint32 _maxSwapPriceImpact) external {
require(msg.sender == owner, 'DXswapFeeReceiver: CALLER_NOT_OWNER');
require(_maxSwapPriceImpact > 0 && _maxSwapPriceImpact < 10000, 'DXswapFeeReceiver: FORBIDDEN_PRICE_IMPACT');
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@karczuRF karczuRF Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maxSwapPriceImpact = _maxSwapPriceImpact;
}

// called by the owner to set external fee receiver address
function setExternalFeeReceiver(address _pair, address _externalReceiver) external {
require(msg.sender == owner, 'DXswapFeeReceiver: CALLER_NOT_OWNER');
externalFeeReceivers[_pair].externalReceiver = _externalReceiver;
}

// 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);
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

address token1 = swapPair.token1();
swapPair.transfer(address(swapPair), feeReceiverBalance);
(uint256 amount0, uint256 amount1) = swapPair.burn(address(this));
if (amount0 > 0) _takeTokenOrETH(address(swapPair), token0, amount0);
if (amount1 > 0) _takeTokenOrETH(address(swapPair), token1, amount1);
emit TakeProtocolFee(msg.sender, ethReceiver, 1);
}
require(swapPair.balanceOf(address(this)) == 0, 'DXswapFeeReceiver: TOKENS_NOT_BURNED');
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// fee percentage check
require(
_feePercentageToExternalReceiver >= 0 && _feePercentageToExternalReceiver <= 5000,
Copy link
Copy Markdown

@luzzif luzzif Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'DXswapFeeReceiver: FORBIDDEN_FEE_PERCENTAGE_SPLIT'
);
// update the split percentage for specific pair
externalFeeReceivers[_pair].feePercentage = _feePercentageToExternalReceiver;
}
}
Loading