RemoteOwner Contract Bricked if setOriginChainOwner is called

[code423n4]

Lines of code

https://github.com/GenerationSoftware/remote-owner/blob/285749ab51e98afc8ebb4e4049a4348d669a3e9d/src/RemoteOwner.sol#L96-L109

Vulnerability details

Impact

Admin calls are verified by the _checkSender(). But there is a function that bricks the contract.

  function setOriginChainOwner(address _newOriginChainOwner) external {
    _checkSender();
    _setOriginChainOwner(_newOriginChainOwner);
  }

This is as a result of the check.

Proof of Concept

  function _checkSender() internal view {
    if (!isTrustedExecutor(msg.sender)) revert LocalSenderNotExecutor(msg.sender);
    if (_fromChainId() != _originChainId) revert OriginChainIdUnsupported(_fromChainId());
    if (_msgSender() != address(_originChainOwner)) revert OriginSenderNotOwner(_msgSender());
  }

If we look at the isTrustedExecutor() function :

  function isTrustedExecutor(address _executor) public view returns (bool) {
    return _executor == trustedExecutor;
  }

where the trustedExecutor is an immutable value set in the constructor.

Therefore, the msg.sender has to be equal to both the trustedExecutor and the _originChainOwner for any calls to the contract to work. If the function setOriginChainOwner() is called, it changes the _originChainOwner but cannot change the trustedExecutor. Hence anytime __checkSender() is called within a function, the call reverts.

  /**
   * @notice Set the OriginChainOwner address.
   * @dev Can only be called once.
   *      If the transaction get front-run at deployment, we can always re-deploy the contract.
   */
  function setOriginChainOwner(address _newOriginChainOwner) external {
    _checkSender();
    _setOriginChainOwner(_newOriginChainOwner);
  }

The code comments says that the function can be called once, But i believe the function should never be called. Both the trustedExecutor and the _originChainOwner have been set in the constructor. Hence changing any value would either brick the contract or revert if they weren't equal previously

Tools Used

Manual Review https://github.com/GenerationSoftware/ERC5164/blob/main/src/abstract/ExecutorAware.sol https://github.com/GenerationSoftware/remote-owner/blob/285749ab51e98afc8ebb4e4049a4348d669a3e9d/src/RemoteOwner.sol

Recommended Mitigation Steps

Remove the function from the contract completely

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as low quality report

[raymondfam]

Invalid assumptions.

[c4-judge]

HickupHH3 marked the issue as duplicate of #40

[HickupHH3]

Therefore, the msg.sender has to be equal to both the trustedExecutor and the _originChainOwner for any calls to the contract to work.

This isn't true; the _msgSender() in

if (_msgSender() != address(_originChainOwner)) revert OriginSenderNotOwner(_msgSender());

is the appended address to the end of msg.data (as per ERC-2771), not msg.sender itself.

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[c4-judge]

HickupHH3 marked the issue as grade-c

[HickupHH3]

154: R