Lines of code
<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/multisigs/GuardCM.sol#L236-L241&gt;
<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/multisigs/GuardCM.sol#L192-L200&gt;
<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/bridges/HomeMediator.sol#L152-L163&gt;
<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/bridges/FxGovernorTunnel.sol#L152-L163&gt;

Vulnerability details

Impact

The GuardCM._verifyBridgedData function is used to verify the bridged data for authorized combinations of targets and selectors in the Gnosis Safe community multisig.

The data payload is passed into the _verifyBridgedData function which is then unpacked inside the for loop by iterating through the bytes of the data payload. The for loop index i is updated accordingly to extract the relevant piece of data from the data payload bytes array as shown below:

    for (uint256 i = 0; i < data.length;) {
        address target;
        uint32 payloadLength;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            // First 20 bytes is the address (160 bits)
            i := add(i, 20) //@audit-info - increase the index i by 20
            target := mload(add(data, i)) //@audit-info - load the data from 20-51 (32 bytes word). Here the last 20 bytes is the target address (the most significant 12 bytes are truncated)
            // Offset the data by 12 bytes of value (96 bits) and by 4 bytes of payload length (32 bits)
            i := add(i, 16) //@audit-info - now the i is 20 + 16 = 36.
            payloadLength := mload(add(data, i)) //@audit-info - load the data from 36-67 (32 bytes word). Here the last 28 bytes is the payload length (the most significant 4 bytes are truncated)
        }

        ...
        ...
    }

As it is evident from the above code snippet the i is increased by 20 and then the target address is extracted from data bytes array (target = data[32:51]). Then the i is increased by 16 and then the payloadLength is extracted from the data bytes array (payloadLength = data[64:67]). As of now the updated value of i is 36 since it has been added 20 and 16 respectively.

Then data validation checks are performed for the target address and the payloadLength values. After that the actual bytes payload is extracted from the data bytes array as shown below:

        bytes memory payload = new bytes(payloadLength); //@audit-info - payload length is the length of the data of the newly created bytes array for the payload being extracted
        for (uint256 j = 0; j < payloadLength; ++j) {
            payload[j] = data[i + j]; //@audit-issue - payload should start from 68th byte onwards. Not from the 36th byte onwards
        }

As it is evident from the above code snippet the actual payload is extracted starting from the data[i+j] byte onwards till the payloadLength is reached. Now let’s consider the first iteration of the above for loop. For the first iteration the j = 0. Hence the i+j = i = 36. Hence payload[0] = data[36]. But this is wrong since the data[36] include a byte which is a part of the target address since the target address data consists from data[32:51].

Hence the payload calculated from the above for loop logic implementation is wrong since it does not use the correct byte index of the data bytes array to extract the payload of the trasnactions correctly.

After the payload is extracted at the end of the inner for loop the i index of the outer for loop is increased as shown below:

        i += payloadLength; 

But here the i is still 36 for the first iteration and the payloadLength will be added to the 36. As a result the resulting i index which is used to extract data for the subsequent transactions are also erroneous. This is because the data[36:67] includes the part of target address, value and payloadLength and the actual payload starts after that. Hence erroneous data will be extracted for the subsequent transactions as well since the payload extraction indexing is wrong.

Hence the GuardCM._verifyData function is called with the wrong payload value at the end. This will prompt the transaction to revert in the _verifyData function since the !mapAllowedTargetSelectorChainIds[targetSelectorChainId] will result in true as shown below:

    uint256 targetSelectorChainId = uint256(uint160(target)); //@audit-info - convert the target into uint160 and then to uint256 where first 160 bits occupy the target
    // selector occupies next 32 bits
    targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160; //@audit-info - 160-191 bits occupy the selector
    // chainId occupies next 64 bits
    targetSelectorChainId |= chainId << 192; //@audit-info - 192-255 bits occupy the chainId
    //@audit-issue - if the chainId ever becomes more than uint64 this contract will not work
    // Check the authorized combination of target and selector
    if (!mapAllowedTargetSelectorChainIds[targetSelectorChainId]) { //@audit-info -  Mapping of (target address | bytes4 selector | uint64 chain Id) => enabled / disabled
        revert NotAuthorized(target, bytes4(data), chainId); //@audit-info - revert if the combination is not authorized
    }

The revert happened since the calculated targetSelectorChainId is wrong and there is not mapping for the targetSelectorChainId key in the mapAllowedTargetSelectorChainIds mapping thus the !mapAllowedTargetSelectorChainIds[targetSelectorChainId] will result in true. Hence the transactino will revert with the NotAuthorized error message.

Hence the GuardCM.checkTransaction transaction (Used to Check the transaction for authorized arguments) will revert since in its execution flow the _verifyBridgedData function is called which will revert due to erroneous data extraction for payload. As a result this will DoS the GuardCM.checkTransaction functionality thus not allowing the GuardCM contract to properly check the scheduling in timelock against the authorized targets and signatures. Hence this breaks a key functionality of the Gnosis Safe community multisig guard.

Similar issues are found in the payload data extractions performed in the HomeMediator.processMessageFromForeign function thus prompting wrong message (payload) to be executed on the target contract as shown below:

        bytes memory payload = new bytes(payloadLength);
        for (uint256 j = 0; j < payloadLength; ++j) {
            payload[j] = data[i + j];
        }
        // Offset the data by the payload number of bytes
        i += payloadLength;

        // Call the target with the provided payload
        (bool success, ) = target.call{value: value}(payload);
        if (!success) {
            revert TargetExecFailed(target, value, payload);
        }

Since multiple transactions are packed into the data bytes array passed to the processMessageFromForeign function, all the transaction executions will be faulty due to errorneous transaction payload extracted.

This could lead to unintended consequences given the implementation of the target contract specially if there is a fallback function is declared in the target contract, since the wrong payload will not call any valid function in the target contract. And due to invalid payload executed on the target if the low-level call function returns false the entire transaction will revert thus not allowing governance actions to be executed on the Gnosis chain.

Similar issue is found in the FxGovernorTunnel.processMessageFromRoot function when the payload is extracted to execute governance actions on Polygon as shown below:

        bytes memory payload = new bytes(payloadLength);
        for (uint256 j = 0; j < payloadLength; ++j) {
            payload[j] = data[i + j];
        }
        // Offset the data by the payload number of bytes
        i += payloadLength;

        // Call the target with the provided payload
        (bool success, ) = target.call{value: value}(payload);
        if (!success) {
            revert TargetExecFailed(target, value, payload);
        }

As a result the target.call transaction will revert since the target is called with the wrong payload extracted. Single revert of one transaction will revert all the transactions since all are packed into the single data bytes array. Hence it would DoS the execution of governance actions on Polygon. This breaks the key functionality of cross-chain governance of the autonolas protocol.

Proof of Concept

        for (uint256 i = 0; i < data.length;) {
            address target;
            uint32 payloadLength;
            // solhint-disable-next-line no-inline-assembly
            assembly {
                // First 20 bytes is the address (160 bits)
                i := add(i, 20)
                target := mload(add(data, i))
                // Offset the data by 12 bytes of value (96 bits) and by 4 bytes of payload length (32 bits)
                i := add(i, 16)
                payloadLength := mload(add(data, i))
            }

<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/multisigs/GuardCM.sol#L212-L223&gt;

            bytes memory payload = new bytes(payloadLength);
            for (uint256 j = 0; j < payloadLength; ++j) {
                payload[j] = data[i + j];
            }
            // Offset the data by the payload number of bytes
            i += payloadLength;

<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/multisigs/GuardCM.sol#L236-L241&gt;

        uint256 targetSelectorChainId = uint256(uint160(target));
        // selector occupies next 32 bits
        targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160;
        // chainId occupies next 64 bits
        targetSelectorChainId |= chainId << 192;

        // Check the authorized combination of target and selector
        if (!mapAllowedTargetSelectorChainIds[targetSelectorChainId]) {
            revert NotAuthorized(target, bytes4(data), chainId);

<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/multisigs/GuardCM.sol#L192-L200&gt;

            bytes memory payload = new bytes(payloadLength);
            for (uint256 j = 0; j < payloadLength; ++j) {
                payload[j] = data[i + j];
            }
            // Offset the data by the payload number of bytes
            i += payloadLength;

            // Call the target with the provided payload
            (bool success, ) = target.call{value: value}(payload);
            if (!success) {
                revert TargetExecFailed(target, value, payload);
            }

<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/bridges/HomeMediator.sol#L152-L163&gt;

            bytes memory payload = new bytes(payloadLength);
            for (uint256 j = 0; j < payloadLength; ++j) {
                payload[j] = data[i + j];
            }
            // Offset the data by the payload number of bytes
            i += payloadLength;

            // Call the target with the provided payload
            (bool success, ) = target.call{value: value}(payload);
            if (!success) {
                revert TargetExecFailed(target, value, payload);
            }

<https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/bridges/FxGovernorTunnel.sol#L152-L163&gt;

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the payload data extractions in the for the specific transactions as shown below:

        bytes memory payload = new bytes(payloadLength);
        for (uint256 j = 0; j < payloadLength; ++j) {
            payload[j] = data[i + 32 + j];
        }

Here the i + 32 + j is used as the index of the data to be extracted into the payload array. This is done since the actual payload of the transaction starts from the 68th byte of the data array and not from the 36th byte as it was erroneously implemented in the logics of the above mentioned respective functions.

Assessed type

DoS

---

The text was updated successfully, but these errors were encountered:

All reactions