PositionManager & PermitERC721 Failure to comply with the EIP-4494

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L42
https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/base/PermitERC721.sol#L77
https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/base/PermitERC721.sol#L13
https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L57

Vulnerability details

Impact

The contract PositionManager.sol inherits from PermitERC721.sol, but both contracts incorrectly implement the EIP-4494 standard, which is an important part of the contract. This leads to the following issues:

• PositionManager & PermitERC721 are not EIP-4494 compliant
• Automatic tools will not be able to determine that this contract has a permit for ERC721
• Third-party contracts will not be able to determine that this is EIP-4494
• Inability to correctly track which nonces are currently relevant, leading to the creation of invalid signatures/signatures for the future
• No support for compact signatures

Proof of Concept

According to the specifications of the standard EIP-4494, the following violations were found:

1. EIP-4494 requires the implementation of IERC165 and the indication of support for the interface 0x5604e225, which is not implemented

2. EIP-4494 requires the presence of the function function nonces(uint256 tokenId) external view returns(uint256); which is missing

3. EIP-4494 requires the function function permit(address spender, uint256 tokenId, uint256 deadline, bytes memory sig) external;, which is incorrectly declared as

File: 2023-05-ajna\ajna-core\src\base\PermitERC721.sol

77:     function permit(
78:         address spender_, uint256 tokenId_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_
79:     ) external {

Tools Used

• Manual review
• Foundry
• https://eips.ethereum.org/EIPS/eip-4494

Recommended Mitigation Steps

• Correct the identified non-compliance issues so that the contracts meet the standard
• Or remove references to the standard and provide a reference to the Uniswap V3 implementation

Assessed type

Other

[c4-sponsor]

MikeHathaway marked the issue as sponsor confirmed

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to Low as part of this is out of scope here, and the rest can be considered an instance of "function incorrect as to spec"

[BhHaipls]

Hi, @Picodes

The ajna-core/src/base/PermitERC721.sol is indeed out of scope, which subsequently makes points 1 and 3 out of scope as well. However, I'd like you to take another look at point 2.

EIP-4494 necessitates the inclusion of the function: function nonces(uint256 tokenId) external view returns(uint256); which is currently absent.

We can see that PermitERC721 is an abstract contract which only partially implements EIP-4494, and it does so incorrectly, thereby making this part out of scope. However, it also imposes a requirement (abstract contract with abstract _getAndIncrementNonce() method) on PositionManager to implement nonces method on its end, which is within scope. This conclusion can be drawn from the following method:

File: ajna-core/src/base/PermitERC721.sol

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

This suggests that the nonces method must be implemented in PositionManager, PermitERC721 transfers this responsibility to descendants. And according to point 2, PositionManager is the final contract that violates the EIP-4494 standard, which is indeed within scope. According to EIP-4494 documentation, the nonces declaration is stated under Three new functions MUST be added to ERC-721:, which, in my opinion, can't be simply dismissed as function incorrect as to spec. According to the practice I've observed, any violation of the standard with MUST label is usually rated as Medium.

Examples:

• DOMAIN_SEPARATOR() is missing in LUSDToken.sol 2023-02-ethos-findings#638
• The tokenURI method does not check if the NFT has been minted and returns data for the contract that may be a fake NFT. 2023-04-caviar-findings#44
• etc...

Thank you

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

@BhHaipls your point is valid: PositionManager should implement nonces. I was reluctant to give this Medium severity as there are issues in PermitERC721 so the contract couldn't implement the EIP anyway, but after reflection and discussing it with an other judge the problem is significant enough to justify Medium severity

[bytes032]

Hey @Picodes,

I would like to address the validity of the issue raised in the following link: #145.

Considering the usage of nonces, which are also employed in the 'permit' functionality, I believe that this issue should be deemed valid. This is particularly relevant because the '_getAndIncrementNonce' function is directly utilized within the 'permit' feature.

It is important to note that PermitERC721 serves as an abstract contract, possessing a critical vulnerability that places an obligation on the PositionManager to implement this vulnerability in its own implementation.

Given that the 'PositionManager' contract does not override the aforementioned function, I contend that it is reasonable to make the same assumption as you did when evaluating this issue. This aligns with the understanding that PermitERC721 shifts the responsibility to its descendants.

Finally, this means permit is a public function that can and will be used by Ajna users who interact with the PositionManager contract, which is in scope.

Thank you for your attention to this matter.

Edit: Here are a few similar issues that have been judged similarly:

1. The call to MsgValueSimulator with non zero msg.value will call to sender itself which will bypass the onlySelf check 2023-03-zksync-findings#153 (comment)
2. https://github.com/code-423n4/2023-04-rubicon-findings/issues/1129.
3. BytesUtil.compare returns wrong result on some strings longer than 32 characters 2022-07-ens-findings#78

That being said, I believe #145 should be downgraded to Medium, given the circumstances.

[Picodes]

@bytes032 I am not sure I understand your argument. How is the signature malleability of PermitERC721 an issue that should have been fixed on PositionManager?
The fact that the final in-scope contract is flawed because of an error in an out-of-scope contract it inherits still makes the error out-of-scope. Otherwise, every dependency would be automatically in-scope?

For nonces, the difference is that as the nonces mapping is in PositionManager, implementing the function was a clear responsibility of PositionManager