Skip to content
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

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

Merged
merged 15 commits into from
May 31, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bde850b
update permiterc721 to comply with 4494 spec
May 15, 2023
ec1f05f
fix build and tests
May 15, 2023
832d145
expand natspec; expand test coverage
May 15, 2023
c00a01f
cleanups and coverage expansion
May 15, 2023
3401525
remove duplicate nonces mapping
May 24, 2023
6aa0cd4
remove unused todo
May 24, 2023
425fe41
Update DOMAIN_SEPARATOR method to external and replace chainId from a…
prateek105 May 26, 2023
775d774
Merge branch 'develop' of github.com:ajna-finance/contracts into ca-p…
May 30, 2023
238c7d0
increment permit nonce on transfer
May 30, 2023
d88f372
update natspec
May 30, 2023
95f75f8
Merge branch 'ca-permit' of github.com:ajna-finance/contracts into ca…
May 30, 2023
3578be8
add test of permit nonce
May 30, 2023
3448c07
parameterize test nonce
May 30, 2023
1b9a6ed
expand permit unit testing
May 30, 2023
c23bb27
expand permit assertions
May 30, 2023
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
17 changes: 2 additions & 15 deletions src/PositionManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R

/// @dev Mapping of `token id => ajna pool address` for which token was minted.
mapping(uint256 => mapping(uint256 => Position)) internal positions;
/// @dev Mapping of `token id => nonce` value used for permit.
mapping(uint256 => uint96) internal nonces;
/// @dev Mapping of `token id => bucket indexes` associated with position.
mapping(uint256 => EnumerableSet.UintSet) internal positionIndexes;

Expand Down Expand Up @@ -128,7 +126,7 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
/**
* @inheritdoc IPositionManagerOwnerActions
* @dev === Write state ===
* @dev `nonces`: remove `tokenId` nonce
* @dev `_nonces`: remove `tokenId` nonce
* @dev `poolKey`: remove `tokenId => pool` mapping
* @dev === Revert on ===
* @dev - `mayInteract`:
Expand All @@ -146,7 +144,7 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
if (positionIndexes[params_.tokenId].length() != 0) revert LiquidityNotRemoved();

// remove permit nonces and pool mapping for burned token
delete nonces[params_.tokenId];
delete _nonces[params_.tokenId];
delete poolKey[params_.tokenId];

_burn(params_.tokenId);
Expand Down Expand Up @@ -396,17 +394,6 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
/*** Internal Functions ***/
/**************************/

/**
* @notice Retrieves token's next nonce for permit.
* @param tokenId_ Address of the `Ajna` pool to retrieve accumulators of.
* @return Incremented token permit nonce.
*/
function _getAndIncrementNonce(
uint256 tokenId_
) internal override returns (uint256) {
return uint256(nonces[tokenId_]++);
}

MikeHathaway marked this conversation as resolved.
Show resolved Hide resolved
/**
* @notice Checks that a provided pool address was deployed by an `Ajna` factory.
* @param pool_ Address of the `Ajna` pool.
Expand Down
255 changes: 196 additions & 59 deletions src/base/PermitERC721.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,124 +2,261 @@

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

pragma solidity 0.8.14;

import { IERC1271 } from '@openzeppelin/contracts/interfaces/IERC1271.sol';
import { ERC721 } from '@openzeppelin/contracts/token/ERC721/ERC721.sol';
import { Address } from '@openzeppelin/contracts/utils/Address.sol';
MikeHathaway marked this conversation as resolved.
Show resolved Hide resolved

import { ERC721 } from '@openzeppelin/contracts/token/ERC721/ERC721.sol';
import { ECDSA } from '@openzeppelin/contracts/utils/cryptography/ECDSA.sol';
MikeHathaway marked this conversation as resolved.
Show resolved Hide resolved
import { SignatureChecker } from '@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol';

/**
* @dev Interface for token permits for ERC-721
*/
interface IPermit {

/**************/
/*** Errors ***/
/**************/

/**
* @notice User queried the nonces of a token that doesn't exxist.
*/
error NonExistentToken();
MikeHathaway marked this conversation as resolved.
Show resolved Hide resolved

/**
* @notice Creator of permit signature is not authorized.
*/
error NotAuthorized();

/**
* @notice User submitted a signature to permit for verification after it's deadline had passed.
*/
error PermitExpired();

/**************************/
/*** External Functions ***/
/**************************/

/**
* @notice `EIP-4494` permit to approve by way of owner signature.
*/
function permit(
address spender_, uint256 tokenId_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_
address spender_, uint256 tokenId_, uint256 deadline_, bytes memory signature_
) external;
}

/**
* @notice https://soliditydeveloper.com/erc721-permit
* @notice Functionality to enable `EIP-4494` permit calls as part of interactions with Position `NFT`s
* @dev spender https://eips.ethereum.org/EIPS/eip-4494
* @dev EIP-4494: https://eips.ethereum.org/EIPS/eip-4494
* @dev References this implementation: https://github.com/dievardump/erc721-with-permits/blob/main/contracts/ERC721WithPermit.sol
*/
abstract contract PermitERC721 is ERC721, IPermit {

/** @dev Gets the current nonce for a token ID and then increments it, returning the original value */
function _getAndIncrementNonce(uint256 tokenId_) internal virtual returns (uint256);
/***************/
/*** Mapping ***/
/***************/

/**
* @dev Mapping of nonces per tokenId
* @dev Nonces are used to make sure the signature can't be replayed
* @dev tokenId => nonce
*/
mapping(uint256 => uint256) internal _nonces;

/*****************/
/*** Constants ***/
/*****************/

/** @dev Value is equal to keccak256("Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)"); */
bytes32 public constant PERMIT_TYPEHASH =
0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad;

/******************/
/*** Immutables ***/
/******************/

// this are saved as immutable for cheap access
// the chainId is also saved to be able to recompute domainSeparator
// in the case of a fork
bytes32 private immutable _domainSeparator;
uint256 private immutable _domainChainId;

/** @dev The hash of the name used in the permit signature verification */
bytes32 private immutable _nameHash;

/** @dev The hash of the version string used in the permit signature verification */
bytes32 private immutable _versionHash;

/** @dev Value is equal to keccak256("Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)"); */
bytes32 public constant PERMIT_TYPEHASH =
0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad;
/*******************/
/*** Constructor ***/
/*******************/

/** @notice Computes the `nameHash` and `versionHash` based upon constructor input */
constructor(
string memory name_, string memory symbol_, string memory version_
) ERC721(name_, symbol_) {
_nameHash = keccak256(bytes(name_));
_versionHash = keccak256(bytes(version_));

// save gas by storing the chainId and DomainSeparator in the state on deployment
_domainChainId = block.chainid;
_domainSeparator = _calculateDomainSeparator(_domainChainId);
}

/************************/
/*** Public Functions ***/
/************************/

/**
* @notice Calculate the `EIP-712` compliant `DOMAIN_SEPERATOR` for ledgible signature encoding.
* @dev The chainID is not set as a constant, to ensure that the chainId will change in the event of a chain fork.
* @return The `bytes32` domain separator of Position `NFT`s.
*/
function DOMAIN_SEPARATOR() public view returns (bytes32) {
return
keccak256(
abi.encode(
// keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)')
0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f,
_nameHash,
_versionHash,
_chainId(),
address(this)
)
);
function DOMAIN_SEPARATOR() external view returns (bytes32) {
return _getDomainSeparator();
}

/**
* @notice Retrieves the current nonce for a given `NFT`.
* @param tokenId_ The id of the `NFT` being queried.
* @return The current nonce for the `NFT`.
*/
function nonces(uint256 tokenId_) external view returns (uint256) {
if (!_exists(tokenId_)) revert NonExistentToken();
return _nonces[tokenId_];
}

/**
* @notice Called by a `NFT` owner to enable a third party spender to interact with their `NFT`.
* @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 v_ Component of `secp256k1` signature.
* @param r_ Component of `secp256k1` signature.
* @param s_ Component of `secp256k1` signature.
* @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(
Copy link
Contributor

@naszam naszam May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@prateek105 prateek105 May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

address spender_, uint256 tokenId_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_
address spender_,
uint256 tokenId_,
uint256 deadline_,
bytes memory signature_
) external {
require(block.timestamp <= deadline_, "ajna/nft-permit-expired");
// check that the permit's deadline hasn't passed
if (block.timestamp > deadline_) revert PermitExpired();

// calculate signature digest
bytes32 digest = _buildDigest(
// owner,
spender_,
tokenId_,
_nonces[tokenId_],
deadline_
);

// check the address recovered from the signature matches the spender
(address recoveredAddress, ) = ECDSA.tryRecover(digest, signature_);
if (!_checkSignature(digest, signature_, recoveredAddress, tokenId_)) revert NotAuthorized();
Comment on lines +152 to +153
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@prateek105 prateek105 May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@naszam naszam May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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


// approve the spender for accessing the tokenId
_approve(spender_, tokenId_);

bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
DOMAIN_SEPARATOR(),
// increment the permit nonce of the given tokenId
_incrementNonce(tokenId_);
Copy link
Contributor

@naszam naszam May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

_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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

/**************************/
/*** Internal Functions ***/
/**************************/

/**
* @notice Builds the permit digest to sign.
* @param spender_ The token spender.
* @param tokenId_ The tokenId.
* @param nonce_ The nonce to make a permit for.
* @param deadline_ The deadline before when the permit can be used.
* @return The digest (following `EIP-712`) to sign.
*/
function _buildDigest(
address spender_,
uint256 tokenId_,
uint256 nonce_,
uint256 deadline_
) internal view returns (bytes32) {
return
ECDSA.toTypedDataHash(
_getDomainSeparator(),
keccak256(
abi.encode(
PERMIT_TYPEHASH,
spender_,
tokenId_,
_getAndIncrementNonce(tokenId_),
nonce_,
deadline_
)
)
)
);
address owner = ownerOf(tokenId_);
require(spender_ != owner, "ERC721Permit: approval to current owner");

if (Address.isContract(owner)) {
// bytes4(keccak256("isValidSignature(bytes32,bytes)") == 0x1626ba7e
require(
IERC1271(owner).isValidSignature(digest, abi.encodePacked(r_, s_, v_)) == 0x1626ba7e,
"ajna/nft-unauthorized"
);
} else {
address recoveredAddress = ecrecover(digest, v_, r_, s_);
require(recoveredAddress != address(0), "ajna/nft-invalid-signature");
require(recoveredAddress == owner, "ajna/nft-unauthorized");
}
}

_approve(spender_, tokenId_);
/**
* @notice Calculate the `EIP-712` compliant `DOMAIN_SEPERATOR` for ledgible signature encoding.
* @dev The chainID is not set as a constant, to ensure that the chainId will change in the event of a chain fork.
* @return The `bytes32` domain separator of Position `NFT`s.
*/
function _getDomainSeparator() internal view returns (bytes32) {
return (block.chainid == _domainChainId) ? _domainSeparator : _calculateDomainSeparator(block.chainid);
}

/**
* @notice Calculates the `EIP-712` compliant `DOMAIN_SEPERATOR` for ledgible signature encoding.
* @param chainId_ The chainId of the network the `NFT` is being interacted with on.
* @return The `bytes32` domain separator of Position `NFT`s.
*/
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)
)
);
}
Comment on lines +206 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@naszam naszam May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@naszam naszam May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

checked the hash via chisel, it matches:

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


/**
* @notice Checks if the recovered address from the signature matches the spender.
* @param digest_ The digest signed by the owner.
* @param signature_ The owner's signature to check.
* @param recoveredAddress_ The address recovered from the signature.
* @param tokenId_ The id of the `NFT` being interacted with.
* @return isValidPermit_ `true` if the recovered address matches the spender, otherwise `false`.
*/
function _checkSignature(
bytes32 digest_,
bytes memory signature_,
address recoveredAddress_,
uint256 tokenId_
) internal view returns (bool isValidPermit_) {
// 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_));
Copy link
Contributor

Choose a reason for hiding this comment

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

See related issue:

Copy link
Contributor

@prateek105 prateek105 May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@naszam

Copy link
Contributor

@naszam naszam May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

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


// else try to recover the signature using SignatureChecker
// this also allows the verifier to recover signatures made via contracts
bool isValidSignature =
SignatureChecker.isValidSignatureNow(
ownerOf(tokenId_),
Copy link
Contributor

@prateek105 prateek105 May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR addresses the prototech issue as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@prateek105 prateek105 May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

See here for related issues:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, above issues are fixed now.

digest_,
signature_
);

isValidPermit_ = (isOwnerOrApproved || isValidSignature);
}

/**
* @dev Gets the current chain id
* @return chainId_ The current chain id
* @notice Increments the nonce for a given `NFT`.
* @param tokenId The id of the `NFT` to increment the nonce for.
*/
function _chainId() internal view returns (uint256 chainId_) {
assembly {
chainId_ := chainid()
}
function _incrementNonce(uint256 tokenId) internal {
_nonces[tokenId]++;
}

}
Loading