QA Report

[code423n4]

In all of proxy pattern contracts (BridgeToken, LPToken, StableSwap, TokenRegistry, PromiseRouter,...) the implementation contract don't initialized

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/LPToken.sol#L13-L27 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/StableSwap.sol#L31-L116

Vulnerability details

Impact

In proxy pattern if the initialization contract don't get initialized then attacker can initialize it and get access to it's admin level functionalities and with logics like self-destruct it can harm the protocol. it's safer to initialize all the implementation contracts too.

Proof of Concept

There is no constructor in contracts to set the state of implementation contract to initialized in BridgeToken, LPToken, StableSwap, TokenRegistry, PromiseRouter and others.. attacker can call initialize() method for implementation contract and take control of it and if there were some logics like self-destruct or ... attacker can use it harm protocol.

Tools Used

VIM

Recommended Mitigation Steps

add constructor and set implementation contract state to initialized

[LayneHaber]

These contracts all have the following modifier attached to the initialize functions:

modifier initializer() {
        bool isTopLevelCall = _setInitializedVersion(1);
        if (isTopLevelCall) {
            _initializing = true;
        }
        _;
        if (isTopLevelCall) {
            _initializing = false;
            emit Initialized(1);
        }
    }

These functions then do update an internal variable marking whether the contract has been initialized to true. The constructors were dropped according to the best practice of writing upgradeable contracts outlined here

[0xleastwood]

While the warden's issue has some truth to it, this would only be a concern if the user who initialized the implementation contract could arbitrarily delegatecall and self-destruct the contract. Because there is no way to directly exploit this, I think it would be fair to downgrade this to QA.

[LayneHaber]

see comment here from previously merged issue!