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

Code Size Optimization: Address.sol imported to use only isContract #30

Closed
naszam opened this issue May 17, 2023 · 1 comment
Closed

Comments

@naszam
Copy link
Collaborator

naszam commented May 17, 2023

Context: PermitERC721.sol#L7

Description:
PermitERC721 imports Address.sol from openzeppelin-contracts library where only use isContract

Recommendation:
Implement directly isContract check in PermitERC721 and remove Address.sol dependency to reduce deployment code size and possibly improve gas efficiency.

diff --git a/src/base/PermitERC721.sol b/src/base/PermitERC721.sol
index a92acaf..cb20ce6 100644
--- a/src/base/PermitERC721.sol
+++ b/src/base/PermitERC721.sol
@@ -4,8 +4,6 @@ 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';
-
 
 /**
  *  @dev Interface for token permits for ERC-721
@@ -97,7 +95,7 @@ abstract contract PermitERC721 is ERC721, IPermit {
         address owner = ownerOf(tokenId_);
         require(spender_ != owner, "ERC721Permit: approval to current owner");
 
-        if (Address.isContract(owner)) {
+        if (owner.code.length > 0) {
             // bytes4(keccak256("isValidSignature(bytes32,bytes)") == 0x1626ba7e
             require(
                 IERC1271(owner).isValidSignature(digest, abi.encodePacked(r_, s_, v_)) == 0x1626ba7e,
  • Test PASS

Ajna:
Removed usage of Address.sol in favor of SignatureChecker

Prototech:
Code changed, adopting SignatureChecker that seems to import Address.sol even if not used.

@kmbarry1
Copy link
Collaborator

kmbarry1 commented Jun 7, 2023

Removing the import in favor of a locally implemented (even inlined) check does not affect contract size in testing (solc is generally smart enough to only import what's actually used, so I'm not surprised by this).

I'm closing this issue, and this finding will not appear in the final report. I added a note on eliminating the Address.sol import just for code readability purposes to #34.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants