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.

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

PositionManager & PermitERC721 implement the EIP-4494 standard, which in DRAFT status #143

Open
code423n4 opened this issue May 8, 2023 · 1 comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

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#L27

Vulnerability details

Impact

Using the EIP-4494 standard in a DRAFT state leads to the possibility that it may change at any moment before reaching Final, or it may never be approved. This, in turn, will result in the PositionManager.sol contract that implements this standard:

  • Not complying with this standard
  • Not complying with the generally accepted standard for permit for ERC721 (another standard may implement this functionality, and this standard will be forgotten)
  • Not being supported by various tools in the future, as they will support the final standard
  • Having the same consequences as in the case of incorrect implementation of the standard

Proof of Concept

We can see that PositionManager follows PermitERC721

File: 2023-05-ajna\ajna-core\src\PositionManager.sol

42: contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, ReentrancyGuard { 

and PermitERC721 implements the EIP-4494 standard

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

10: /**
11:  *  @dev Interface for token permits for ERC-721
12:  */
13: interface IPermit {
14:     /**
15:     *  @notice `EIP-4494` permit to approve by way of owner signature.
16:     */
17:     function permit(
18:         address spender_, uint256 tokenId_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_
19:     ) external;
20: }

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

23:  *  @notice https://soliditydeveloper.com/erc721-permit
24:  *  @notice Functionality to enable `EIP-4494` permit calls as part of interactions with Position `NFT`s
25:  *  @dev    spender https://eips.ethereum.org/EIPS/eip-4494
26:  */
27: abstract contract PermitERC721 is ERC721, IPermit {

To check the status of the standard: https://eips.ethereum.org/EIPS/eip-4494

Draft - The first formally tracked stage of an EIP in development. An EIP is merged by an EIP Editor into the EIP repository when properly formatted.

Tools Used

Recommended Mitigation Steps

  • Remove references to the standard, and indicate that this is an ERC721-Permit implementation from Uniswap V3

Assessed type

ERC721

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 8, 2023
code423n4 added a commit that referenced this issue May 8, 2023
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 18, 2023
@c4-judge
Copy link
Contributor

Picodes changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added the QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax label May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

3 participants