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

[audit] #6: Cleanup expiry/ownership logic in BaseRegistrar #52

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

stevieraykatz
Copy link
Collaborator

From Spearbit:

Description
Reuse the implementation from solady for better optimisation and also refactor the non-expiry check into a modifier.

Recommendation
Apply the following patch:

diff --git a/src/L2/BaseRegistrar.sol b/src/L2/BaseRegistrar.sol
index 823beef..e170339 100644
--- a/src/L2/BaseRegistrar.sol
+++ b/src/L2/BaseRegistrar.sol
@@ -137,6 +137,14 @@ contract BaseRegistrar is ERC721, Ownable {
         _;
     }
 
+    /// @notice Decorator for determining if a name is not expired.
+    ///
+    /// @param id The id being checked for non-expiry.
+    modifier onlyNonExpired(uint256 id) {
+        if (nameExpires[id] <= block.timestamp) revert Expired(id);
+        _;
+    }
+
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
     /*                        IMPLEMENTATION                      */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
@@ -234,8 +242,7 @@ contract BaseRegistrar is ERC721, Ownable {
     /// @param tokenId The id of the name to query the owner of.
     ///
     /// @return address The address currently marked as the owner of the given token ID.
-    function ownerOf(uint256 tokenId) public view override returns (address) {
-        if (nameExpires[tokenId] <= block.timestamp) revert Expired(tokenId);
+    function ownerOf(uint256 tokenId) public view override onlyNonExpired(tokenId) returns (address) {
         return super.ownerOf(tokenId);
     }
 
@@ -374,8 +381,13 @@ contract BaseRegistrar is ERC721, Ownable {
     ///
     /// @return `true` if msg.sender is approved for the given token ID, is an operator of the owner,
     ///         or is the owner of the token, else `false`.
-    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view override returns (bool) {
-        address owner = ownerOf(tokenId);
-        return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
+    function _isApprovedOrOwner(address spender, uint256 tokenId)
+        internal
+        view
+        override
+        onlyNonExpired(tokenId)
+        returns (bool)
+    {
+        return super._isApprovedOrOwner(spender, tokenId);
     }
 }

@stevieraykatz stevieraykatz merged commit 0a89eac into main Jul 9, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant