code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Upgradable.sol#L29-L33 https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Upgradable.sol#L78-L80 https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Proxy.sol#L40-L43

Vulnerability details

Impact

The Upgradeable.onlyProxy modifier is used to ensure that a function can only be called by the proxy and can not be directly called in the Upgradeable.sol contract.

The onlyProxy modifier implementation is as follows:

modifier onlyProxy() {
    // Prevent setup from being called on the implementation
    if (address(this) == implementationAddress) revert NotProxy();
    _;
}

Only the proxy contract can call a function guarded this modifier via a delegatecall. If directly called this modifier will revert the transaction.

This modifier is used in the Upgradeable.setup function. The setup function is used to setup the implementation contract with initial data.

The problem here is that a malicious user can create his own Proxy Contract and delegatecall the implementation contract by calling its Upgradeable.setup function. This way he can bypass the onlyProxy modifier since now the address(this) == implementationAddress condition will be false and the transaction will execute.

This issue is aggravated since there is no initializer modifier (from openzeppelin) in the Upgradeable.setup function. Hence this function can be called multiple times.

As a result the malicious user can use the above vulnerabilities to delegatecall the Upgradeable.setup with his own setupParams (after the setup is called via the Proxy.constructor) and change the state of the implementation contract and push it into an undesirable state.

For example the InterchainTokenService contract inherits from Upgradeable contract. The InterchainTokenService._setup function is used to call the Operatable._setOperator function to set the operator_ address of the InterchainTokenService. Hence an attacker can use the above mentioned attack path to set the operator to a malicious address and control the InterchainTokenService.setFlowLimit function (controlled by onlyOperator modifier) to set his own flow limits for token managers thus violating the contract state.

Thus the users who use this compromised implementation contract will be susceptible to undesirable transactions.

Proof of Concept

    modifier onlyProxy() {
        // Prevent setup from being called on the implementation
        if (address(this) == implementationAddress) revert NotProxy();
        _;
    }

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Upgradable.sol#L29-L33

    function setup(bytes calldata data) external override onlyProxy {
        _setup(data);
    }

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Upgradable.sol#L78-L80

        if (setupParams.length != 0) {
            (bool success, ) = implementationAddress.delegatecall(abi.encodeWithSelector(IUpgradable.setup.selector, setupParams));
            if (!success) revert SetupFailed();
        }

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/gmp-sdk/upgradable/Proxy.sol#L40-L43

Tools Used

Manaul Review and VSCode

Recommended Mitigation Steps

Since the Upgradeable.setup function is used to setup the implementation contract with initial data (as per the Natspec comments), it is recommended to add initializer modifier to the setup function so it can be only called once for initialization by the proxy contract (inside its constructor during deployment). So an attacker will not be able to call the setup function via a malicious proxy contract and change the state of the implementation contract later.