onlyCentrifugeChainOrigin() can't require msg.sender equal axelarGateway

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/gateway/routers/axelar/Router.sol#L44

Vulnerability details

Vulnerability details

In AxelarRouter.sol, we need to ensure the legitimacy of the execute() method execution, mainly through two methods

1. axelarGateway.validateContractCall () to validate if the command is approved or not
2. onlyCentrifugeChainOrigin() is used to validate that sourceChain sourceAddress is legal.

Let's look at the implementation of onlyCentrifugeChainOrigin()

    modifier onlyCentrifugeChainOrigin(string calldata sourceChain, string calldata sourceAddress) {        
@>      require(msg.sender == address(axelarGateway), "AxelarRouter/invalid-origin");
        require(
            keccak256(bytes(axelarCentrifugeChainId)) == keccak256(bytes(sourceChain)),
            "AxelarRouter/invalid-source-chain"
        );
        require(
            keccak256(bytes(axelarCentrifugeChainAddress)) == keccak256(bytes(sourceAddress)),
            "AxelarRouter/invalid-source-address"
        );
        _;
    }

The problem is that this restriction msg.sender == address(axelarGateway)

When we look at the official axelarGateway.sol contract, it doesn't provide any call external contract 'sexecute() method

so msg.sender cannot be axelarGateway, and the official example does not restrict msg.sender

the security of the command can be guaranteed by axelarGateway.validateContractCall(), sourceChain, sourceAddress.

there is no need to restrict msg.sender

axelarGateway code address https://github.com/axelarnetwork/axelar-cgp-solidity/blob/main/contracts/AxelarGateway.sol

can't find anything that calls router.execute()

Impact

router.execute() cannot be executed properly, resulting in commands from other chains not being executed， protocol not working properly

Recommended Mitigation

remove msg.sender restriction

    modifier onlyCentrifugeChainOrigin(string calldata sourceChain, string calldata sourceAddress) {        
-       require(msg.sender == address(axelarGateway), "AxelarRouter/invalid-origin");
        require(
            keccak256(bytes(axelarCentrifugeChainId)) == keccak256(bytes(sourceChain)),
            "AxelarRouter/invalid-source-chain"
        );
        require(
            keccak256(bytes(axelarCentrifugeChainAddress)) == keccak256(bytes(sourceAddress)),
            "AxelarRouter/invalid-source-address"
        );
        _;
    }

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx (sponsor) confirmed

[gzeon-c4]

This seems High risk to me since the Axelar bridge is a centerpiece of this protocol, and when deployed in a certain way where the AxelarRouter is the only ward, it might causes user deposits to be stuck forever.

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[gzeon-c4]

Reconsidering severity to Med here since the expected setup would have DelayedAdmin able to unstuck the system

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[c4-judge]

gzeon-c4 marked the issue as selected for report

[merteren1234]

Hi @gzeon-c4 i think this issue should be high. Because if this issue will not fix than protocol will suffer %100 percent after launch and without deploy new contract this issue will not fix. Let assume this contract deploy without fix this issue: Than in short time it will be understood that execute function cannot be used and critical functions of protocol cannot be used and this happens without any malicious act or edge case or protocol teams mistake. After that protocol team needs to pause everything should give permission to unstake tokens from protocol (which lead to centralization risk) and deploy new gateawat router contract and this situation will effect protocol's repetuation and relability for users.I think this issue has high impact due to critical severity of posibility and not impact just some cases of users but effect all protocol's core functions.

[gzeon-c4]

The protocol would not be able to whitelist user by calling updateMember if this is deployed, so the issue is likely to be discovered very early and can be redeployed without chance of user fund getting stuck.

[merteren1234]

@gzeon-c4 Thanks for answer. yes this issue would discovered before users use this protocol. But my main point while giving 'what if' scenario is show that how big impact of the this vulnerability .Because this vulnerability makes whole system unusable. Also this vulnerability happens not rely to possibility. %100 system will suffer from this.Because of that i think this issue should high instead of medium.

[gzeon-c4]

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements. 3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Since asset is not at risk and the mistake can be easily discovered early with a easy fix to reconfigure the gateway using the delayed admin, I think Med is appropriate as it only impact the availability of the protocol temporarily.

[merteren1234]

hmm i get it. Thanks for clarification.

[hieronx]

Mitigated in https://github.com/centrifuge/liquidity-pools/pull/168