keccak123 - Owner has multiple meanings when proxy is involved

[github-actions]

keccak123

low

Owner has multiple meanings when proxy is involved

Summary

The word "owner" is overloaded and refers to more than one address in the proxies used in optimism bedrock.

Vulnerability Detail

The proxy contract used for several contracts, including L1CrossDomainMessenger, confuses the meaning of owner in the code and in the spec. In the proxy contract, there is OWNER_KEY, but this storage slot actually stores the admin of the proxy and is retrieved by calling admin. This meaning of owner is more confusing because L1CrossDomainMessenger, the implementation contract behind the proxy, inherits OwnableUpgradeable and has an owner function, a transferOwnership function, and an onlyOwner modifier.

The overloading of "owner" makes all documentation with the word "owner" confusing, and sometimes contradictory, in the specifications and code natspec. One specific example of this contradiction is from the predeploy spec

ProxyAdmin Address: 0x4200000000000000000000000000000000000018

The ProxyAdmin is the owner of all of the proxy contracts set at the predeploys. It is itself behind a proxy. The owner of the ProxyAdmin will have the ability to upgrade any of the other predeploy contracts.

The first time the word "owner" is used, it actually refers to the proxy admin. This is seen by calling admin on L2CrossDomainMessenger: cast call 0x4200000000000000000000000000000000000007 "admin()(address)" --rpc-url https://goerli.optimism.io -> 0x4200000000000000000000000000000000000018. Using the same admin meaning for owner, like the first time the word is used, returns the ProxyAdmin address, which does not make sense in the context of what the spec is trying to explain: cast call 0x4200000000000000000000000000000000000018 "admin()(address)" --rpc-url https://goerli.optimism.io -> 0x4200000000000000000000000000000000000018. But the second time the word "owner" is used, it refers to the owner behind the proxy, in the implementation contract, which is seen in this cast call: cast call 0x4200000000000000000000000000000000000018 "owner()(address)" --rpc-url https://goerli.optimism.io -> 0xf80267194936da1E98dB10bcE06F3147D580a62e. This explanation of the spec should replace the first time the word "owner" is used with the word "admin".

Impact

The meaning of owner is ambiguous in many contracts because there owner can refer to the return value of admin or the return value of owner.

Code Snippet

The natspec for changeAdmin and admin in the proxy shows how the word owner is used to refer to the proxy admin, confusing the two terms
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/Proxy.sol#L111-L126

The natspec and variable name in L1CrossDomainMessenger uses owner to refer to a different value
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L1CrossDomainMessenger.sol#L38

The predeploy spec for ProxyAdmin summarizes this confusion by using the word "owner" two times to refer to different values
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/predeploys.md#proxyadmin

Tool used

Manual Review

Recommendation

Disambiguate the terms admin and owner. The proxyadmin spec explanation should replace the first time the word "owner" is used with the word "admin". Consider renaming OWNER_KEY in the proxy contract to ADMIN_KEY and remove the word owner from the proxy contract.