Centrifuge - merlin's results

Lines of code

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

Vulnerability details

Bug Description

The AxelarRouter is an important smart contract that operates as follows:

1. The Gateway smart contract encodes outgoing messages and sends them to the AxelarRouter using the AxelarRouter.send function. The AxelarRouter then forwards the message to the centrifugeGatewayPrecompileAddress on the Centrifuge chain through Axelar's General Message Passing mechanism.
2. The AxelarRouter smart contract receives incoming messages through the AxelarRouter.execute function, which is processed by Centrifuge using Axelar's General Message Passing.

In the AxelarRouter smart contract, the AxelarRouter.execute function has a modifier that checks whether msg.sender is an AxelarGateway smart contract. However, msg.sender can never be an AxelarGateway address.

modifier onlyCentrifugeChainOrigin(
        string calldata sourceChain,
        string calldata sourceAddress
    ) {
        require(
            msg.sender == address(axelarGateway),
            "AxelarRouter/invalid-origin"
        );
        // requires
        _;
    }

Proof-of-Concept

The process unfolds as follows: AxelarRouter --> AxelarGateway --> CentrifugeGateway --> AxelarGateway --> AxelarRouter

An example to illustrate this issue:

1. Bob initiates any function that sends a message to the CentrifugeGateway smart contract on the Centrifuge chain through Axelar's General Message Passing, such as LiquidityPool.requestDeposit function.
2. The CentrifugeGateway forwards the message to the AxelarGateway on the Centrifuge chain.
3. The call is approved within the AxelarGateway on the destination chain.
4. At this stage, the executor service relays and executes the approved call to the AxelarRouter smart contract. However, the AxelarGateway smart contract lacks the logic to invoke the AxelarRouter.execute function on AxelarRouter smart contract.

Note: Centrifuge bot pays for all executing functions automatically.

As a result, all incoming messages from the CentrifugeGateway will not be executed. You can verify this by inspecting the AxelarGateway smart contract. You can also review AxelarScan (INTERCHAIN TRANSACTIONS) at this link. Specifically, open any transaction with the status executed and observe that there is no call from AxelarGateway to the execute function in any smart contract.

In simple terms, the approval of a contract call occurs through AxelarGateway. After that, when calling the execute function in the AxelarRouter smart contract, the validateContractCall is checked. However, msg.sender can never be an AxelarGateway address.

Impact

All incoming messages from Centrifuge chain to AxelarRouter smart contract will fail.

Tools Used

Manual

Recommended Mitigation Steps

To solve this issue, you need to remove the require:

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"
        );
        _;
    }

After that, anyone will be able to call the AxelarRouter.execute function. It is protected by a validateContractCall check that only allows execution if this function returns true and the AxelarGateway marks the message as executed so that the contract call is not executed twice.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L377 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L390 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L403 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L416 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L350-L367 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L612

Vulnerability details

Impact

Other protocols that integrate with Centrifuge may wrongly assume that the functions are EIP-4626 compliant. Thus, it might cause integration problems in the future that can lead to wide range of issues for both parties.

Proof-of-Concept

All official EIP-4626 requirements can be found on it's official page. Non-compliant functions are listed below along with the reason they are not compliant:

The following functions are non-compliant because they don't account for smart contract being paused:

1. LiquidityPool.maxDeposit()
2. LiquidityPool.maxMint()
3. LiquidityPool.maxWithdraw()
4. LiquidityPool.maxRedeem()

MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0

The following functions are non-compliant because they account for limits like those returned from maxDeposit, maxMint, maxWithdraw, maxRedeem:

1. LiquidityPool.previewDeposit()
2. LiquidityPool.previewMint()
3. LiquidityPool.previewWithdraw()
4. LiquidityPool.previewRedeem()

MUST NOT account for deposit limits like those returned from maxDeposit and should always act as though the deposit would be accepted, regardless if the user has enough tokens approved, etc.

Following are the expected rounding direction for each function:

previewMint(uint256 shares) - Round Up ⬆ previewWithdraw(uint256 assets) - Round Up ⬆ previewRedeem(uint256 shares) - Round Down ⬇ previewDeposit(uint256 assets) - Round Down ⬇ convertToAssets(uint256 shares) - Round Down ⬇ convertToShares(uint256 assets) - Round Down ⬇

Following is the OpenZeppelin's ERC4626.sol implementation for rounding reference. (link)

Tools Used

Manual

Recommended Mitigation Steps

All functions listed above should be modified to meet the specifications of EIP-4626.

Assessed type

ERC4626

The Messages._stringToBytes128() and _stringToBytes32 does not verify whether the string source argument exceeds bytes in length.

To address this concern, a require statement is used to validate that the input string's length (bytes(source).length) does not exceed 128 bytes. If the string's length exceeds 128 bytes, the transaction is reverted, accompanied by the error message "String exceeds 128 bytes." This approach ensures the safe conversion of the input string without data loss, as the function only continues if the length requirement is met, and it terminates the transaction if the requirement is not met.

function _stringToBytes128(
        string memory source
    ) internal pure returns (bytes memory) {
+       require(bytes(source).length <= 128, "String exceeds 128 bytes");
        bytes memory temp = bytes(source);
        bytes memory result = new bytes(128);

        // for loop

        return result;
    }

function _stringToBytes32(
        string memory source
    ) internal pure returns (bytes32 result) {
+       require(bytes(source).length <= 32, "String exceeds 128 bytes");
        bytes memory tempEmptyStringTest = bytes(source);
        if (tempEmptyStringTest.length == 0) {
            return 0x0;
        }

        assembly {
            result := mload(add(source, 32))
        }
    }

Useless liquidityPool argument in InvestmentManager._toPriceDecimals function

Consider removing the useless liquidityPool argument in the InvestmentManager._toPriceDecimals function.

function _toPriceDecimals(
        uint128 _value,
        uint8 decimals,
-        address liquidityPool
    ) internal view returns (uint256 value) {
        if (PRICE_DECIMALS == decimals) return uint256(_value);
        value = uint256(_value) * 10 ** (PRICE_DECIMALS - decimals);
    }