Anyone Can Become Owner Of `XC20Wrapper` Contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L11 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/Upgradable.sol#L66 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L34

Vulnerability details

Anyone can become the owner of the XC20Wrapper contract by calling the XC20Wrapper.setup function.

Proof-of-Concept

The XC20Wrapper contract inherits from Upgradable contract.

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L11

contract XC20Wrapper is AxelarExecutable, Upgradable {

As such, the XC20Wrapper contract inherits the setup function from the Upgradable contract.

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/Upgradable.sol#L66

function setup(bytes calldata data) external override {
    // Prevent setup from being called on the implementation
    if (implementation() == address(0)) revert NotProxy();

    _setup(data);
}

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L34

function _setup(bytes calldata data) internal override {
    (address owner_, bytes32 codehash_) = abi.decode(data, (address, bytes32));
    _transferOwnership(owner_);
    xc20Codehash = codehash_;
}

Anyone can call the XC20Wrapper.setup function because there is no access control except for preventing it from being called from the implementation. The XC20Wrapper.setup function will in turn call the _transferOwnership function which will transfer the ownership to the address specified within the data parameter.

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/Upgradable.sol#L29

function _transferOwnership(address newOwner) internal {
    if (newOwner == address(0)) revert InvalidOwner();

    emit OwnershipTransferred(newOwner);
    // solhint-disable-next-line no-inline-assembly
    assembly {
        sstore(_OWNER_SLOT, newOwner)
    }
}

Thus, an attacker can call the XC20Wrapper.setup function and craft a data that allows them to become the owner.

Impact

Attacker gains full control of the XC20Wrapper contract as the owner. An attacker could possibly remove the unwrapping feature by calling XC20Wrapper.removeWrapping for all the tokens and cause users to be unable to unwrap their tokens. This effectively makes the wrapped tokens useless as there is no way to unwrap them to get back the original token.

Recommendation

Implement access control on the setup function and ensure that only the owner can call this function.

+ function setup(bytes calldata data) external onlyOwner override {
- function setup(bytes calldata data) external override {
    // Prevent setup from being called on the implementation
    if (implementation() == address(0)) revert NotProxy();

    _setup(data);
}

[re1ro]

Not applicable. if (implementation() == address(0)) revert NotProxy(); does all the job. Plus shadowing setup defined in Proxy

[GalloDaSballo]

Dup of #5

[re1ro]

Duplicate of #87

[re1ro]

Duplicate of #5