Fix for tokens that charge fees. #1636
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
+ Coverage 85.94% 85.99% +0.04%
==========================================
Files 21 21
Lines 925 928 +3
Branches 162 163 +1
==========================================
+ Hits 795 798 +3
Misses 104 104
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I noticed the event: Is unused through out our code. I am wondering if we should not emit it in the |
contracts/src/Functions.sol
Outdated
| if (balanceAfter < balanceBefore) { | ||
| revert InvalidAmount(); | ||
| } | ||
|
|
||
| uint256 delta = balanceAfter - balanceBefore; | ||
| if (delta == 0 || delta > type(uint128).max) { | ||
| revert InvalidAmount(); | ||
| } | ||
|
|
||
| transferredAmount = uint128(delta); |
There was a problem hiding this comment.
Another option would be to simply revert if the delta does not match the expected amount. Notably, DeFi protocols like Uniswap do not support these edge cases either. https://docs.uniswap.org/concepts/protocol/integration-issues
There was a problem hiding this comment.
I like this, just revert and not support these kinds of tokens at all. @vgeddes ?
The only bad thing with this route, since we have already accepted PAXG, is that if they enabled fees on transfer and it reverted if we detected that. Then all PAXG would essentially never be allowed to be unlocked.
There was a problem hiding this comment.
So I have analyzed a large portion of bridged transfers from mainet(10k, ~80%), and we have not run this case. So I think it would be safe to what you suggest and revert @yrong.
There was a problem hiding this comment.
Agreed, just revert if delta != amount
That is redundant with the standard ERC-20 Transfer event. |
contracts/src/Functions.sol
Outdated
| revert InvalidAmount(); | ||
| } | ||
|
|
||
| transferredAmount = uint128(delta); |
There was a problem hiding this comment.
Doesn't seem like this return value is ever used.
There was a problem hiding this comment.
Yeah I used to used to emit an event on a previous revision d0c2d5c.
That used the return value to emit an event. But then removed it because:
#1636 (comment)
But it still seems useful to have it as a return value of a function.
alistair-singh
force-pushed
the
alistair/fix-fee-tokens
branch
from
0501689 to
49bdba9
Compare
contracts/src/Functions.sol
Outdated
| uint256 balanceBefore = IERC20(token).balanceOf(recipient); | ||
| bytes memory call = abi.encodeCall(AgentExecutor.transferToken, (token, recipient, amount)); | ||
| invokeOnAgent(agent, executor, call); | ||
|
|
||
| uint256 balanceAfter = IERC20(token).balanceOf(recipient); | ||
| if (balanceAfter != balanceBefore + amount) { | ||
| revert TransferAmountMismatch(); | ||
| } |
There was a problem hiding this comment.
it doesn't make sense to revert when unlocking user funds, since the main reason for this PR is to prevent a mismatch between locked funds on the source, and minted funds on the destination.
We don't care about the opposite direction.
contracts/src/Functions.sol
Outdated
| uint256 balanceAfter = IERC20(token).balanceOf(agent); | ||
| if (balanceAfter != balanceBefore + amount) { | ||
| revert TransferAmountMismatch(); |
There was a problem hiding this comment.
simplification:
| uint256 balanceAfter = IERC20(token).balanceOf(agent); | |
| if (balanceAfter != balanceBefore + amount) { | |
| revert TransferAmountMismatch(); | |
| if (IERC20(token).balanceOf(agent) != balanceBefore + amount) { | |
| // Tokens with Fee-On-Transfer behaviour are not supported | |
| revert InvalidToken(); |
alistair-singh
force-pushed
the
alistair/fix-fee-tokens
branch
from
49bdba9 to
f3b4caa
Compare
Resolves: SNO-1650