SOV-4106 Feat: Fee sharing collector with direct transfer #548
Conversation
cwsnt
changed the title
There was a problem hiding this comment.
Generally LGTM, added a couple of comments about concerns:
- Changing the native token address constant value is a possible footgun if the contract is updated on RSK
- Make sure changing the ABI doesn't break frontend / backend!
- I don't understand how it worked before the onlyExecution modifier was added, but I probably just missed something
contracts/governance/FeeSharingCollector/FeeSharingCollector.sol
Outdated
Show resolved
Hide resolved
contracts/governance/FeeSharingCollector/FeeSharingCollector.sol
Outdated
Show resolved
Hide resolved
| // We will change, so that feeSharingCollector will directly burn then loanToken (IWRBTC) to rbtc and send to the user --- by call burnToBTC function | ||
| ILoanTokenWRBTC(_token).burnToBTC(_receiver, amount, false); | ||
| if (loanWrappedNativeTokenAddress == _token) { | ||
| // We will change, so that feeSharingCollector will directly burn then loanWrappedNativeToken (IWrappedNativeToken) to nativeToken and send to the user --- by call burnToBTC function |
There was a problem hiding this comment.
| // We will change, so that feeSharingCollector will directly burn then loanWrappedNativeToken (IWrappedNativeToken) to nativeToken and send to the user --- by call burnToBTC function | |
| // We will change, so that feeSharingCollector will directly burn then loanWrappedNativeToken (IWrappedNativeToken) to nativeToken and send to the user --- by call burnToBTC function which is burning to a native token - to be renamed to burnedToNativeToken |
There was a problem hiding this comment.
addressed in this commit
| } else { | ||
| // Previously it directly send the loanToken to the user | ||
| // Previously it directly send the loanWrappedNativeToken to the user |
There was a problem hiding this comment.
why is the loanToken renamed to loanWrappedNativeToken?
There was a problem hiding this comment.
it was accidentally replaced, reverted back in this addressed in this commit
| * the fees (except SOV) will be converted in wRBTC form, and then will be transferred to wRBTC loan pool. | ||
| * For SOV, it will be directly deposited into the FeeSharingCollectorMultipleToken from the protocol. |
There was a problem hiding this comment.
| * the fees (except SOV) will be converted in wRBTC form, and then will be transferred to wRBTC loan pool. | |
| * For SOV, it will be directly deposited into the FeeSharingCollectorMultipleToken from the protocol. |
| ); | ||
| } | ||
|
|
||
| uint256 wrbtcAmountWithdrawn = protocol.withdrawFees(_tokens, address(this)); |
There was a problem hiding this comment.
why does this multitoken implementation doesn't have RBTC mentions renamed to native token?
| } | ||
|
|
||
| /** | ||
| * @notice Withdraw fees from sovryn dex: |
There was a problem hiding this comment.
| * @notice Withdraw fees from sovryn dex: | |
| * @notice Withdraw fees from Sovryn DEX |
|
|
||
| if (loanWrappedNativeToken == _token) { | ||
| // We will change, so that FeeSharingCollectorMultipleToken will directly burn then loanToken (IWRBTC) to rbtc and send to the user --- by call burnToBTC function | ||
| uint256 loanAmountPaid = ILoanWrappedNativeToken(_token).burnToBTC( |
| false | ||
| ); | ||
| } else { | ||
| // Previously it directly send the token to the user |
There was a problem hiding this comment.
that's what it does now
| // Previously it directly send the token to the user |
| * | ||
| * @param _tokens array addresses of the tokens | ||
| * */ | ||
| function withdrawFeesFromDex(address[] memory _tokens) public { |
There was a problem hiding this comment.
missing tests for this function
| require(_amount > 0, "FeeSharingCollectorMultipleToken::transferTokens: invalid amount"); | ||
|
|
||
| /// @notice Transfer tokens from msg.sender | ||
| bool success = IERC20(_token).transferFrom(address(msg.sender), address(this), _amount); |
There was a problem hiding this comment.
it will fail when processing the native token - should check the value payload and process separately.
There was a problem hiding this comment.
i don't think the initial function of transferTokens here is supporting native token?
|
|
||
| /** | ||
| * @notice Transfer tokens to this contract. | ||
| * @dev We just update amount of tokens here and write checkpoint in a separate methods |
There was a problem hiding this comment.
pls add a @dev note that the caller should take care of setting allowance for the token
| contract FeeSharingCollectorMultiToken is | ||
| SafeMath96, | ||
| IFeeSharingCollectorMultiToken, | ||
| Ownable, |
There was a problem hiding this comment.
Using Ownable instead of OwnableUpgradeable, but I discussed with Cowsant that this has been the norm for upgradeable contracts.
| * | ||
| * @param _tokens array address of the token | ||
| * */ | ||
| function withdrawFees(address[] memory _tokens) public { |
There was a problem hiding this comment.
Is there any risk in this being public and re-entrant? Also this function doesn't validate _tokens, but maybe it happens in protocol.withdrawFees?
| * the fees will be converted in wrappedNativeToken form, and then will be transferred to wrappedNativeToken loan pool | ||
| * | ||
| * */ | ||
| function withdrawFeesFromLBDex() public { |
There was a problem hiding this comment.
Any risk with this being public and re-entrant?
|
|
||
| /// @notice Transfer tokens from msg.sender | ||
| bool success = IERC20(_token).transferFrom(address(msg.sender), address(this), _amount); | ||
| require(success, "Staking::transferTokens: token transfer failed"); |
There was a problem hiding this comment.
Staking::transferTokens? Should this be FeeSharingCollectorMultiToken::transferTokens?
| sender: root, | ||
| token: loanWrappedNativeToken.address, | ||
| amount: new BN(0), | ||
| }); |
There was a problem hiding this comment.
@cwsnt this test seems weird: shouldn't we have an "expectRevert" checking here, instead of an event of a successful tx?
| ); | ||
| }); | ||
|
|
||
| it("Shouldn't be able to withdraw zero amount", async () => { |
There was a problem hiding this comment.
@cwsnt Wrong test description. Instead of "Shouldn't be able to withdraw zero amount", should say: "Shouldn't be able to withdraw above available"
No description provided.