feat: support selfPermitForERC721 #62
Conversation
| uint8 v, | ||
| bytes32 r, | ||
| bytes32 s | ||
| ) external payable override { |
There was a problem hiding this comment.
No need payable since this doesn't use msg.value.
There was a problem hiding this comment.
i remember if we remove payable here, multicall will fail if msg.value is non-zero ( consider the case when user call migrateFromV2 with some extra ETH ).
There was a problem hiding this comment.
agree with Burger, though if possible maybe can add a simple test, so it fail if someone remove the payable 🙏
There was a problem hiding this comment.
test added, thx for reminding
| payable | ||
| override | ||
| { | ||
| IERC721Permit(token).permit(address(this), tokenId, deadline, v, r, s); |
There was a problem hiding this comment.
I think this has the same issue as https://www.trust-security.xyz/post/permission-denied
_getAndIncrementNonce will increment the nonce, so here's how a griefing to cause the user tx to revert is done.
- User signs a valid signature and calls selfPermitERC721
- Attacker frontruns with the signature and calls
IERC721Permit(token).permit(address(this), tokenId, deadline, v, r, s); - Attacker uses the signature successfully and causes the user's nonce to increase
- User's tx revert as the nonce has already been increase and does not match the nonce used for the signature.
There was a problem hiding this comment.
Oh, i got what u mean, so would u recommend i call selfPermitERC721IfNecessary here ?
There was a problem hiding this comment.
I took a second look and I think selfPermitERC721IfNecessary would still revert if the user if frontrun once, but after that, if selfPermitERC721IfNecessary is used again (although not necessary since the approve is done by the frontrun), it will work.
There was a problem hiding this comment.
Sorry, i don't understand why is not working at the first time. I am thinking the workflow as such:
- User offchain sign and send the tx
- Attacker monitors the tx in mem-pool and frontruns it
- Now the nonce has increased and our migrator contract has been approved to do operation on the NFT
- User's tx arrives but since migrator has already been approved, if body will be skipped.
There was a problem hiding this comment.
Ah yes, it will skip at step 4. It will be ok if selfPermitERC721IfNecessary if always used then
There was a problem hiding this comment.
Can write a comment in the code to always use selfPermitERC721IfNecessary then
chefburger
force-pushed
the
feat/supportSelfPermitForERC721
branch
from
4b9b8d4 to
7a702f3
Compare
chefburger
force-pushed
the
feat/supportSelfPermitForERC721
branch
from
7a702f3 to
5b2a93a
Compare
* feat: impl binPool migrator * refactor: restructure & renaming accordingly * test: add tests cases for binPool migrator * chore: renaming as well to align with clMigrator * fix: add check to prevent token mismatch between source and target pool * feat: added refundETH function and necessary comments * optimization: avoid duplicate external call when query v2/v3 pool info * feat: support selfPermitForERC721 (#62) * feat: support selfPermitForERC721 * docs: added comments suggesting users to use selfPermitERC721IfNecessary * test: added tests to prevent ppl from removing payable keyword from external functions
* feat: implement v4Migrator for cl-pool * feat: added same price slippage check for migrateFromV2 * feat: using actual receive amount when adding liquidity to v4 * test: added test for clMigrator * fix: take into consideration of extra funds when calc refund * optimization: avoid unnecessary approve in old token cases * optimization: avoid unnecessary refund call after adding liquidity * optimization: avoid unnecessary WETH unwrap * fix: extra check so that non-owner can not steal funds from lpV3 token * typo * refactor: restructure baseMigrator a bit so that more can be reused across pool type * chore: typo and renaming * V4 BinMigrator (#59) * feat: impl binPool migrator * refactor: restructure & renaming accordingly * test: add tests cases for binPool migrator * chore: renaming as well to align with clMigrator * fix: add check to prevent token mismatch between source and target pool * feat: added refundETH function and necessary comments * optimization: avoid duplicate external call when query v2/v3 pool info * feat: support selfPermitForERC721 (#62) * feat: support selfPermitForERC721 * docs: added comments suggesting users to use selfPermitERC721IfNecessary * test: added tests to prevent ppl from removing payable keyword from external functions * docs: add explanation about the case where extra token0 is ETH
It provides SelfPermitERC721 extension support for migrator because v3 LP Token supports PermitERC721 extension. This is extremely helpful if user want to approve through offchain signature