Admin accounts with critical permissions cannot be set to multi-sig wallets at initialization

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L247
https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L169
https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L147
https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L165

Vulnerability details

Impact

Accounts with critical permissions should be controlled by multi-sig wallets. However, the initialization methods of the listed contracts don't allow setting the initial admin address to a multi-sig.

Proof of Concept

Some contracts use tx.origin (origin() in assembly syntax) to initialize the admin accounts. Since tx.origin refers to the first address in the sequence of calls, we know it belongs to an externally owned account (EOA). This is dangerous because a single person can exercise all the associated privileges alone. In particular, any person with access to the original EOA can gain exclusive access by setting the admin address to an account he controls alone. Notice, however, that the admin can also transfer the privileges to a multi-sig after initialization.

Further, notice that the Admin.setAdmin function does not protect against the zero address and does not implement a two-phase transferal, which can lead to the accidental loss of all admin privileges.

Tools Used

VSCode

Recommended Mitigation Steps

Don't use tx.origin to initialize the admin address. It could be turned into a parameter instead. Make sure to align the deployment process accordingly.

[gzeoneth]

As mentioned, it can easily be transferred afterward.

[alexanderattar]

This is a design decision that deployer key is not security key. The deployment pipeline is setup for deployer key to deploy contracts and pass all rights/ownership to multi-sig