Codearena-145,147: PermitERC721 EIP-4494 compliance #818

MikeHathaway · 2023-05-15T22:18:24Z

Description of change

High level

Update PermitERC721 to be compliant with EIP-4494
- Based upon the reference implementation here: https://github.com/dievardump/erc721-with-permits/blob/main/contracts/ERC721WithPermit.sol

Description of bug or vulnerability and solution

MikeHathaway · 2023-05-15T23:15:55Z

src/base/PermitERC721.sol

@@ -2,124 +2,264 @@



Does the license need to be switched from BUSL-1.1 since it uses some logic from a MIT licensed codebase?

No, the MIT license doesn't even require derivative works to be open-sourced. If we have used a "substantial portion" of that implementation, however, we should include a copyright notice in this file. However, the original work by dievardump does not include a LICENSE file or copyright notice.

prateek105 · 2023-05-16T14:11:05Z

src/base/PermitERC721.sol

+        // this also allows the verifier to recover signatures made via contracts
+        bool isValidSignature =
+            SignatureChecker.isValidSignatureNow(
+                ownerOf(tokenId_),


I think this does not cover the case where the signature is made by a contract that is not the owner of the tokenId but instead is approved by the owner.

check also this one Fixed-Point-Solutions/prototech-ajna-audit#27

This PR addresses the prototech issue as well

Also, I believe that this specific case is checked on line 1367 of PositionManager.t.sol @prateek105

I saw the tests and the scenario I mentioned is different from the test, my scenario will fail for contracts but pass for EOA. Scenario is that an EOA account which is approved can give permit to spender address on behalf of owner but the same thing will fail for a contract which is approved trying to give permit to a spender. The same limitation applies to this reference implementation https://github.com/dievardump/erc721-with-permits/blob/main/contracts/ERC721WithPermit.sol as well.

I think that this is the limitation of ERC1271, so I am approving the PR.

See here for related issues:

isValidSignature doesn't check for well formed signature Fixed-Point-Solutions/prototech-ajna-audit#33

Code Improvement: consider implementing isValidSignature directly Fixed-Point-Solutions/prototech-ajna-audit#35

I believe the usage of OpenZeppelin's SignatureChecker library should fix the previous issues with isValidSignature.

yes, above issues are fixed now.

prateek105

looks good to me. added a comment here.

src/base/PermitERC721.sol

naszam · 2023-05-23T13:03:00Z

src/base/PermitERC721.sol

+
+        // get chainId for the domain
+        uint256 chainId;
+        //solhint-disable-next-line no-inline-assembly
+        assembly {
+            chainId := chainid()
+        }
+
+        // save gas by storing the chainId and DomainSeparator in the state on deployment
+        _domainChainId = chainId;
+        _domainSeparator = _calculateDomainSeparator(chainId);


See here for related issues (gas optimization and domain separator internal discussion):

Gas Optimization: consider using block.chainid instead of inline assembly chainid Fixed-Point-Solutions/prototech-ajna-audit#28

src/base/*: Pool.sol, FlashloanablePool.sol, PermitERC721.sol, PoolDeployer.sol Fixed-Point-Solutions/prototech-ajna-audit#1 (comment)
cc: @kmbarry1

Fixed in PR #850

src/base/PermitERC721.sol

naszam · 2023-05-23T13:21:15Z

src/base/PermitERC721.sol

+        // verify if the recovered address is owner or approved on tokenId
+        // and make sure recoveredAddress is not address(0), else getApproved(tokenId) might match
+        bool isOwnerOrApproved =
+            (recoveredAddress_ != address(0) && _isApprovedOrOwner(recoveredAddress_, tokenId_));


See related issue:

Gas Optimization: consider removing redundant check in PermitERC721 Fixed-Point-Solutions/prototech-ajna-audit#36

~~yes, right, the zero address check is redundant I can add a commit for this.~~

zero address check is required as getApproved(tokenId) can return zero address

getApproved(tokenId) uses the _exists check, inside _requireMinted, to ensure the tokenId exists and checks also to ensure owner is not address zero (in oz v4.8.2):

getApproved

_exists

But I see that for the approved address there doesn't seem to be any check and it could return indeed a zero address and considering the refactoring with the extra logic it makes sense now to have an explicit check for zero address due to the extra conditions, isApprovedForAll(owner, spender) and getApproved(tokenId) == spender (the above issue was related to the previous implementation specifically).

src/PositionManager.sol

naszam · 2023-05-25T16:13:00Z

src/base/PermitERC721.sol

+        (address recoveredAddress, ) = ECDSA.tryRecover(digest, signature_);
+        if (!_checkSignature(digest, signature_, recoveredAddress, tokenId_)) revert NotAuthorized();


Why these extra steps are introduced instead of using directly isValidSignatureNow check from SignatureChecker.sol?
In this issue I've broken down the recommendation with related issues for consideration:

Code Improvement: consider implementing isValidSignature directly Fixed-Point-Solutions/prototech-ajna-audit#35

Primarily to offload as much of the logic as possible to well audited and tested external libraries. I recognize the gas and storage size benefits to just implementing that logic directly, I had just avoided it to minimize the amount of logic that is self-rolled.

another reason we can not use isValidSignatureNow is that this check returns just the bool. we want to get the recovered address because the recovered address can wither be owner or the approved account. if we use isValidSignatureNow permit will fail when the approved account(not the owner) has given the permit. This check here, _isApprovedOrOwner(recoveredAddress_, tokenId_)
@naszam

I just noticed now that the refactoring is following the implementation linked in the description, I'll check that in depth and adjust the certora specs accordingly (the related issues linked were referring to the previous implementation, so they might need to be adjusted following the linked implementation where the _isApprovedOrOwner function seems to be required, thanks for flagging that out).

…ssembly with block.chainid (#850)

EdNoepel · 2023-05-29T15:58:39Z

src/base/PermitERC721.sol

@@ -2,124 +2,264 @@



No, the MIT license doesn't even require derivative works to be open-sourced. If we have used a "substantial portion" of that implementation, however, we should include a copyright notice in this file. However, the original work by dievardump does not include a LICENSE file or copyright notice.

naszam · 2023-05-30T12:43:58Z

src/base/PermitERC721.sol

-                "\x19\x01",
-                DOMAIN_SEPARATOR(),
+        // increment the permit nonce of the given tokenId
+        _incrementNonce(tokenId_);


_incrementeNonce seems to be done inside the overrided _transfer in the reference linked implementation (see here), in accordance with the standard specifications:

In general, test suites should assert at least the following about any implementation of this EIP:

the nonce is incremented after each transfer

permit approves the spender on the correct tokenId

the permit cannot be used after the NFT is transferred

an expired permit cannot be used

Alternatively, usually is before the approval in the erc20 permit.
What is the rationale behind this change?

That was a mistake on my part, no solid rationale in light of @prateek105 proposed exploit. Updated to _incrementNonce in an override _transfer similar to the reference implementation.

…ermit

…-permit

grandizzy · 2023-05-31T11:48:46Z

@naszam could you please re review this one, should be ready to merge

naszam · 2023-05-31T12:05:21Z

src/base/PermitERC721.sol

+     *  @param spender_   The address of the third party who will execute the transaction involving an owners `NFT`.
+     *  @param tokenId_   The id of the `NFT` being interacted with.
+     *  @param deadline_  The unix timestamp by which the permit must be called.
+     *  @param signature_ The owner's permit signature to verify.
     */
    function permit(


just checking if you've considered to include the overloaded permit as well, as follow:

function permit( address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s ) external { permit(spender, tokenId, deadline, abi.encodePacked(r, s, v)); }

It doesn't seem to be mentioned in the standard implementation but I guess would be good to have for integrators.

i think it's a small task for integrators to generate signature at their end, i m in favour of not adding this method in contract as it adds one more step for the permit increasing gas cost as well as increases contract size.

naszam · 2023-05-31T13:05:17Z

src/base/PermitERC721.sol

+    function _calculateDomainSeparator(uint256 chainId_) internal view returns (bytes32) {
+        return
+            keccak256(
+                abi.encode(
+                    // keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)')
+                    0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f,
+                    _nameHash,
+                    _versionHash,
+                    chainId_,
+                    address(this)
+                )
+            );
+    }


This seems to diverge with the standard implementation, where the chainId_ is used in the keccak256 as follows:

function _calculateDomainSeparator(uint256 chainId) internal view returns (bytes32) { return keccak256( abi.encode( keccak256( 'EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)' ), keccak256(bytes(name())), keccak256(bytes('1')), block.chainid, address(this) ) ); }

where indeed the chainId_ passed as argument can be used directly instead of block.chainid, but the hash I guess it should be computed each time as block.chainid might diverge from deployment, due to forks.

As per my understanding of this comment, u mean that we should calculate keccak256( 'EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)' ) each time rather than hardcoding it as 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f

But this is hash of a string which is constant across forks, so we can hardcode this instead of calculating it each time.

If u meant something else please let me know.

Oh! Right, got confused with the argument, good to resolve, I was just ensuring block.chainid is passed each time the internal function is called.

checked the hash via chisel, it matches:

➜ bytes32 hash3= keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)') ➜ hash3 Type: bytes32 └ Data: 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f

naszam

Diffed against Issue 27, related issues and standard reference implementation

LGTM

update permiterc721 to comply with 4494 spec

bde850b

MikeHathaway changed the base branch from master to develop May 15, 2023 22:18

fix build and tests

ec1f05f

MikeHathaway marked this pull request as ready for review May 15, 2023 22:59

MikeHathaway requested review from prateek105, ith-harvey, grandizzy and EdNoepel May 15, 2023 22:59

expand natspec; expand test coverage

832d145

MikeHathaway commented May 15, 2023

View reviewed changes

MikeHathaway added Priority: Critical Scope: Bug fix Scope: Style/Consistency labels May 15, 2023

cleanups and coverage expansion

c00a01f

MikeHathaway added Scope: Edits /src Status: Ready for Review labels May 15, 2023

prateek105 reviewed May 16, 2023

View reviewed changes

grandizzy changed the title ~~Code Arena: PermitERC721 EIP-4494 compliance~~ Codearena-145,147: PermitERC721 EIP-4494 compliance May 17, 2023

prateek105 approved these changes May 19, 2023

View reviewed changes

kmbarry1 mentioned this pull request May 21, 2023

ERC Compliance: PermitERC721 and PositionManager implementations doesn't match with ERC-4494 Draft Fixed-Point-Solutions/prototech-ajna-audit#27

Open

grandizzy reviewed May 23, 2023

View reviewed changes

src/base/PermitERC721.sol Show resolved Hide resolved

src/base/PermitERC721.sol Outdated Show resolved Hide resolved

grandizzy requested a review from naszam May 23, 2023 09:35

naszam reviewed May 23, 2023

View reviewed changes

src/base/PermitERC721.sol Show resolved Hide resolved

naszam reviewed May 23, 2023

View reviewed changes

src/base/PermitERC721.sol Show resolved Hide resolved

naszam reviewed May 23, 2023

View reviewed changes

src/PositionManager.sol Show resolved Hide resolved

Mike added 2 commits May 23, 2023 22:04

remove duplicate nonces mapping

3401525

remove unused todo

6aa0cd4

naszam mentioned this pull request May 24, 2023

Code Size Optimization: Address.sol imported to use only isContract Fixed-Point-Solutions/prototech-ajna-audit#30

Closed

naszam mentioned this pull request May 24, 2023

Code Inconsistency: PermitERC721 uses require where elsewhere custom errors are adopted Fixed-Point-Solutions/prototech-ajna-audit#29

Open

naszam reviewed May 25, 2023

View reviewed changes

Update DOMAIN_SEPARATOR method to external and replace chainId from a…

425fe41

…ssembly with block.chainid (#850)

EdNoepel approved these changes May 29, 2023

View reviewed changes

naszam reviewed May 30, 2023

View reviewed changes

Mike added 8 commits May 30, 2023 15:27

Merge branch 'develop' of github.com:ajna-finance/contracts into ca-p…

775d774

…ermit

increment permit nonce on transfer

238c7d0

update natspec

d88f372

Merge branch 'ca-permit' of github.com:ajna-finance/contracts into ca…

95f75f8

…-permit

add test of permit nonce

3578be8

parameterize test nonce

3448c07

expand permit unit testing

1b9a6ed

expand permit assertions

c23bb27

grandizzy requested a review from naszam May 31, 2023 11:48

naszam reviewed May 31, 2023

View reviewed changes

MikeHathaway merged commit b36bf64 into develop May 31, 2023

MikeHathaway deleted the ca-permit branch May 31, 2023 16:08

naszam approved these changes May 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codearena-145,147: PermitERC721 EIP-4494 compliance #818

Codearena-145,147: PermitERC721 EIP-4494 compliance #818

MikeHathaway commented May 15, 2023 •

edited by ith-harvey

Loading

MikeHathaway May 15, 2023

EdNoepel May 29, 2023

prateek105 May 16, 2023 •

edited

Loading

grandizzy May 16, 2023

MikeHathaway May 16, 2023

MikeHathaway May 16, 2023

prateek105 May 19, 2023 •

edited

Loading

naszam May 23, 2023

MikeHathaway May 24, 2023

prateek105 May 24, 2023

prateek105 left a comment

naszam May 23, 2023 •

edited

Loading

prateek105 May 24, 2023

naszam May 23, 2023

prateek105 May 26, 2023 •

edited

Loading

prateek105 May 26, 2023

prateek105 May 30, 2023

naszam May 30, 2023 •

edited

Loading

naszam May 25, 2023

MikeHathaway May 25, 2023

prateek105 May 26, 2023 •

edited

Loading

naszam May 29, 2023 •

edited

Loading

EdNoepel May 29, 2023

naszam May 30, 2023 •

edited

Loading

MikeHathaway May 30, 2023

grandizzy commented May 31, 2023

naszam May 31, 2023 •

edited

Loading

prateek105 May 31, 2023 •

edited

Loading

naszam May 31, 2023

prateek105 May 31, 2023

naszam May 31, 2023 •

edited

Loading

naszam May 31, 2023 •

edited

Loading

naszam left a comment

		(address recoveredAddress, ) = ECDSA.tryRecover(digest, signature_);
		if (!_checkSignature(digest, signature_, recoveredAddress, tokenId_)) revert NotAuthorized();

Codearena-145,147: PermitERC721 EIP-4494 compliance #818

Codearena-145,147: PermitERC721 EIP-4494 compliance #818

Conversation

MikeHathaway commented May 15, 2023 • edited by ith-harvey Loading

Description of change

High level

Description of bug or vulnerability and solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 left a comment

Choose a reason for hiding this comment

naszam May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naszam May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

naszam May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naszam May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented May 31, 2023

naszam May 31, 2023 • edited Loading

Choose a reason for hiding this comment

prateek105 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naszam May 31, 2023 • edited Loading

Choose a reason for hiding this comment

naszam May 31, 2023 • edited Loading

Choose a reason for hiding this comment

naszam left a comment

Choose a reason for hiding this comment

MikeHathaway commented May 15, 2023 •

edited by ith-harvey

Loading

prateek105 May 16, 2023 •

edited

Loading

prateek105 May 19, 2023 •

edited

Loading

naszam May 23, 2023 •

edited

Loading

prateek105 May 26, 2023 •

edited

Loading

naszam May 30, 2023 •

edited

Loading

prateek105 May 26, 2023 •

edited

Loading

naszam May 29, 2023 •

edited

Loading

naszam May 30, 2023 •

edited

Loading

naszam May 31, 2023 •

edited

Loading

prateek105 May 31, 2023 •

edited

Loading

naszam May 31, 2023 •

edited

Loading

naszam May 31, 2023 •

edited

Loading