base-org / basenames

Base-native Identity
https://www.base.org/names
MIT License
44 stars 12 forks source link

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

Closed stevieraykatz closed 3 months ago

stevieraykatz commented 4 months ago

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);
     }
 }