Trusted forwarder can be malicious and can attack `onlyOwner` functionality.

[hats-bug-reporter[bot]]

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x5bf6295a209931aa704ef98d6eda1ce43ad6c24b2b4e878baa0c0f2bad440167 Severity: high

Description: Description\

Trusted forwarder can be malicious and can attack onlyOwner functionality.

Attack Scenario\

ModuleFactory contract implements ERC-2771 that use a trust forwarder to create new module on behalf of the users of the protocol. The problem is that the trust forwarder can basically update the beacon metadata instead of the owner of the contract. In the current implementation, there is an override of the ERC-2771 _msgSender() function that actually allows the trusted forwarder to update the value of msg.sender to anybody he wants:

https://github.com/InverterNetwork/contracts/blob/main/src/factories/ModuleFactory.sol#L165-173

function _msgSender()
        internal
        view
        virtual
        override(ERC2771Context, Context)
        returns (address sender)
    {
        return ERC2771Context._msgSender();
    }

onlyOwner() modifier, in its turn, relies on _msgSender():

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L48-51

modifier onlyOwner() {
        _checkOwner();
        _;
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L63-67

function _checkOwner() internal view virtual {
        if (owner() != _msgSender()) {
            revert OwnableUnauthorizedAccount(_msgSender());
        }
    }

onlyOwner modifier is used in a crucial function that registers beacon metadata:

https://github.com/InverterNetwork/contracts/blob/main/src/factories/ModuleFactory.sol#L143-146

function registerMetadata(
        IModule.Metadata memory metadata,
        IInverterBeacon beacon
    ) external onlyOwner validMetadata(metadata) validBeacon(beacon)

Therefore, a trusted forwarder can basically take control of the crucial functionality that is supposed to be used only by the owner.

Recommendation\ Restrict the trusted forwarder from the ability to use the only owner functionality.

[PlamenTSV]

1

[0xmahdirostami]

thank you @PlamenTSV