ProjectOpenSea / operator-filter-registry

MIT License
312 stars 93 forks source link

The check on from and msg.sender should be outside the codesize check #47

Closed dievardump closed 1 year ago

dievardump commented 1 year ago

https://github.com/ProjectOpenSea/operator-filter-registry/blob/529cceeda9f5f8e28812c20042cc57626f784718/src/OperatorFilterer.sol#L39-L51

The cost of comparing from and msg.sender is way less than the cost to go check the code size of an external address.

For the sake of not wasting gas on direct transfers (and imo a better flow) this code could be rewritten into

        if (from != msg.sender) {
          // Check registry code length to facilitate testing in environments without a deployed registry.
          if (address(OPERATOR_FILTER_REGISTRY).code.length > 0) {
              if (!OPERATOR_FILTER_REGISTRY.isOperatorAllowed(address(this), msg.sender)) {
                  revert OperatorNotAllowed(msg.sender);
              }
          }
        }
        _;

this would also allow to put the checking snippet in its own internal

function _checkOperatorFilter(address operator) internal virtual {
  if (address(OPERATOR_FILTER_REGISTRY).code.length > 0) {
      if (!OPERATOR_FILTER_REGISTRY.isOperatorAllowed(address(this), operator)) {
          revert OperatorNotAllowed(operator);
      }
  }
}

and have a cleaner code reuse

    modifier onlyAllowedOperator(address from) virtual {
       if (from != msg.sender) {
         _checkOperatorFilter(msg.sender);
       }
        _;
    }

    modifier onlyAllowedOperatorApproval(address operator) virtual {
        _checkOperatorFilter(operator);
        _;
    }

    function _checkOperatorFilter(address operator) internal virtual {
      // Check registry code length to facilitate testing in environments without a deployed registry.
      if (address(OPERATOR_FILTER_REGISTRY).code.length > 0) {
          if (!OPERATOR_FILTER_REGISTRY.isOperatorAllowed(address(this), operator)) {
              revert OperatorNotAllowed(operator);
          }
      }
    }
}

This would also allow to facilitate overrides from implementers, for example to add a switchs to enable/disable the OperatorFilter when all this drama of not wanting to respect creator fees/royalties will stop Adding ~150gas at execution, but makes it hell of easier to override