initialize functions might be invoked multiple of times

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/factories/ArbitrumBranchBridgeAgentFactory.sol#L54-L65 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/CoreRootRouter.sol#L63-L67 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootPort.sol#L128-L155

Vulnerability details

Impact

Function initialize might be invoked multiple of times. A malicious owner (or the malicious actor who compromised contracts' owner keys) can re-initialize contracts values as many times as he/she wants.

The issue affects three contracts: ArbitrumBranchBridgeAgentFactory.sol, CoreRootRouter.sol, RootPort.sol.

The severity has been estimated as Medium, since malicious owner (likehood: Low) can enforce crucial changes which will break contract (impact: High) by reinitializing its values.

Proof of Concept

File: ulysses-omnichain/factories/ArbitrumBranchBridgeAgentFactory.sol

54      function initialize(address _coreRootBridgeAgent) external override onlyOwner {
55          address newCoreBridgeAgent = address(
56              DeployArbitrumBranchBridgeAgent.deploy(
57                  wrappedNativeToken,
58                  rootChainId,
59                  _coreRootBridgeAgent,
60                  localAnyCallAddress,
61                  localAnyCallExecutorAddress,
62                  localCoreBranchRouterAddress,
63                  localPortAddress
64              )
65          );
66
67          IPort(localPortAddress).addBridgeAgent(newCoreBridgeAgent);
68      }

initialize can be run multiple of times.

File: ulysses-omnichain/CoreRootRouter.sol

63      function initialize(address _bridgeAgentAddress, address _hTokenFactory) external onlyOwner {
64          bridgeAgentAddress = payable(_bridgeAgentAddress);
65          bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
66          hTokenFactoryAddress = _hTokenFactory;
67      }

initialize can be run multiple of times. It allows the malicious owner to set new bridgeAgentAddress, bridgeAgentExecutorAddress and hTokenFactoryAddress

File: ulysses-omnichain/RootPort.sol

128      function initialize(address _bridgeAgentFactory, address _coreRootRouter) external onlyOwner {
129          require(_setup, "Setup ended.");
130          require(_bridgeAgentFactory != address(0), "Bridge Agent Factory cannot be 0 address.");
131          require(_coreRootRouter != address(0), "Core Root Router cannot be 0 address.");
132
133          isBridgeAgentFactory[_bridgeAgentFactory] = true;
134          bridgeAgentFactories.push(_bridgeAgentFactory);
135          bridgeAgentFactoriesLenght++;
136
137          coreRootRouterAddress = _coreRootRouter;
138      }
139
140      function initializeCore(
141          address _coreRootBridgeAgent,
142          address _coreLocalBranchBridgeAgent,
143          address _localBranchPortAddress
144      ) external onlyOwner {
145          require(_setup, "Setup ended.");
146          require(isBridgeAgent[_coreRootBridgeAgent], "Core Bridge Agent doesn't exist.");
147          require(_coreRootBridgeAgent != address(0), "Core Root Bridge Agent cannot be 0 address.");
148          require(_coreLocalBranchBridgeAgent != address(0), "Core Local Branch Bridge Agent cannot be 0 address.");
149          require(_localBranchPortAddress != address(0), "Local Branch Port Address cannot be 0 address.");
150
151          coreRootBridgeAgentAddress = _coreRootBridgeAgent;
152          localBranchPortAddress = _localBranchPortAddress;
153          IBridgeAgent(_coreRootBridgeAgent).syncBranchBridgeAgent(_coreLocalBranchBridgeAgent, localChainId);
154          getBridgeAgentManager[_coreRootBridgeAgent] = owner();
155      }

Both initialize and initializeCore can be run multiple of times - unless owner runs function forefeitOwnership, which sets _setup to false.

Tools Used

Manual code review

Recommended Mitigation Steps

The main recommendation is to use the modifier initializer. For ulysses-omnichain/factories/ArbitrumBranchBridgeAgentFactory.sol, the fix could be the same as it's done in ulysses-omnichain/factories/BranchBridgeAgentFactory.sol, where renounceOwnership(); is being added as last instruction inside initialize.

Assessed type

Other

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c

[0xLightt]

Addressed https://github.com/Maia-DAO/eco-c4-contest/commit/896757dd4b6f6254a5de05709283d568e8da5e34