CCCM EVM V1 - Concrete

It has been observed that the extractSubset function is using an incorrect temp.lastFrgmtCnt value, set as 0x0001000000002000188101000000200018.

// insert the instructions and make a low level call.
            (bool callSuccess, bytes memory newResponsesEncoded) = address(this).call(
                abi.encodeWithSelector(
                    this.multipleReceivePseudoInternal.selector,
                    numFrgmt,
                    temp.lastFrgmtCnt, // last fragment from the previous bundle
                    packets_[0].sliceRevertOnInvalid(temp.startByte, endByte), // relevant part of instructions
                    CccmLib.extractSubset(packets_, temp.lastFrgmtCnt, numFrgmt),
                    responses,
                    successFlags
                )
            );

Here, temp.lastFrgmtCnt is incorrectly selecting packets_[0], which corresponds to the instructions. As a result, modifiedPacket is set incorrectly to the hexadecimal values of the instructions, rather than targeting the appropriate packet for performing the external call on the multipleReceivePseudoInternal function.

The following Foundry test was used in order to prove the aforementioned issue:

function test_cccmMultiReceiveWithInstructions() public {
        mockCallContract.createSecondMockCallContract();
        MockCallContractSimple secondMockCallContract = MockCallContractSimple(mockCallContract.getSecondMockAddress());
        bytes[] memory packets = new bytes[](5);
        packets[0] = abi.encodePacked(
            bytes1(0x00), // 00000000 => flag = 0 and length = 0. First packet.
            bytes1(0x81), // 00000001 => flag = 0 and length = 1. Second packet. Takes de response from first packet.
            hex"00", // index of the first response
            hex"0000", // start at the beginning of the response
            hex"0020", // length of the cropped response (= 32 bytes)
            hex"0018", // byte where the cropped response data should be inserted (24 = 20 + 4 = 0x18)
            bytes1(0x81), // 10000001 => flag = 1 and length = 1. 
            hex"01", // index of the second response
            hex"0000", // start at the beginning of the response
            hex"0020", // length of the cropped response
            hex"0018", // byte where the cropped response data should be inserted
            bytes1(0x82), // 10000010 => flag = 0 and length = 2
            hex"02", // index of the third response (i.e. the address of the mock contract)
            hex"000c", // start at the beginning of the response
            hex"0014", // length of the cropped response (length of an address, i.e. "0x14"=20 bytes)
            hex"0000", // byte where the cropped response data should be inserted
            hex"01", // index of the second response
            hex"0000", // start at the beginning of the response
            hex"0020", // length of the cropped response
            hex"0018" // byte where the cropped response data should be inserted
        );

        // bytes responses[] = new bytes[](4);
        // read the instruction for the first packet. There are none. But the flag is 0, so this is NOT the last packet in the bundle.
        // (bool success, bytes responses[0]) = address(packets[1][0:20]).call(packets[1][20:]])
        // read the instruction for the second packet. There is one. The flag is again 1, so it is INDEED the last packet in the bundle (first bundle). The instructions says: take the response from the first packet (i.e. index 0 = hex"00"), then crop it from the 0th byte (0x00 = 0) and take 32 bytes (0x20 = 32), in other words that would be the entire response (i.e. uint256(B) = 33).
        // and paste it into the calldata. I.e. into packets[2], like so:
        // modifiedPacket = packets[2][0:24] + responses[0][0:32] + packets[2][24 + 32:]  (actually the last part is empty, because the packet[2] only has 56 bytes = address + selector + uint256)
        // NOTE that the original data that was in the call data is now lost. So the number 6 is lost.
        // (bool success, bytes responses[1]) = address(modifiedPacket[0:20]).call(modifiedPacket[20:]])
        // read the instruction for the third packet. There is one. The flag is 0, so it is NOT the last packet in the bundle.
        // The instructions says: take the response from the first packet (i.e. index 1 = hex"01"), then crop it from byte 0 (0x00 = 0) and take 32 bytes (0x20 = 32), in other words that would be the entire responses[1] (i.e. uint256(A + B) = 45). And paste it into the calldata. I.e. into packets[3], like so:
        // modifiedPacket = packets[3][0:24] + responses[1][0:32] + packets[3][24 + 32:]  (actually the last part is empty, because the packet[3] only has 56 bytes = address + selector + uint256)
        // NOTE that the original data that was in the call data is now lost. So the number 0 is lost.
        // (bool success, bytes responses[2]) = address(modifiedPacket[0:20]).call(modifiedPacket[20:]])
        // read the instruction for the fourth packet. There are two. The flag is 1, so it is INDEED the last packet in the bundle. The second bundle.
        // The first instruction says take the response from the third packet (i.e. index 2 = hex"02"), then crop it from byte 0 (0x00 = 0) and take 20 bytes (0x14 = 20), in other words that would be the entire address of the second mock contract. And paste it into the calldata. I.e. into packets[4], like so:
        // modifiedPacket = responses[2][0:20] + packets[4][20:]
        // Then take the response from the second packet (i.e. index 1 = hex"01"), then crop it from byte 0 (0x00 = 0) and take 32 bytes (0x20 = 32), in other words that would be the entire responses[1] (i.e. uint256(A + B) = 45). And paste it into the calldata. I.e. into packets[4], like so:
        // modifiedPacket = modifiedPacket[0:20] + responses[1][0:32] + packets[4][20 + 32:]  (actually the last part is empty, because the packet[4] only has 52 bytes = address + selector + uint256)
        packets[1] = abi.encodePacked(address(mockCallContract), MockCallContract.getB.selector); // call number = 0  => response index = 0
        packets[2] = abi.encodePacked(address(mockCallContract), MockCallContract.addToA.selector, uint256(6));
        packets[3] = abi.encodePacked(address(mockCallContract), MockCallContract.getAddress.selector, uint256(0));
        packets[4] = abi.encodePacked(address(mockCallContract), MockCallContract.addToA.selector, uint256(6));
        // expected responses
        bytes[] memory responses = new bytes[](4);
        responses[0] = abi.encode(B);
        responses[1] = abi.encode(A + B);
        responses[2] = abi.encode(address(secondMockCallContract));
        responses[3] = abi.encode(A + B);
        // no response template
        bytes memory responseTemplate = "";
        // send the packets without instructions and expect event to be emitted
        vm.startPrank(executor);
        uint224 nonce = uint224(multiMessaging.getNonce());
        bytes32 packetId = bytes32((uint256(nonce) << 32) | uint256(targetEid));
        // create a uint256 from the binary format 001111
        uint256 successFlags = 1 + 2 + 4 + 8;
        // check if the event was emitted
        vm.expectEmit(true, true, true, true, address(multiMessaging));
        emit EndpointMultiMessageEvents.MultiResponse(packetId, keccak256(abi.encode(packets)), successFlags, responses);
        multiMessaging.cccmMultiReceive(packetId, eid, packets, responseTemplate);
        vm.stopPrank();
    }

Evidence:

Consider incrementing one to temp.lastFrgmtCnt value in order to use the target address once it decoded instead of instructions.

// insert the instructions and make a low level call.
            (bool callSuccess, bytes memory newResponsesEncoded) = address(this).call(
                abi.encodeWithSelector(
                    this.multipleReceivePseudoInternal.selector,
                    numFrgmt,
                    temp.lastFrgmtCnt, // last fragment from the previous bundle
                    packets_[0].sliceRevertOnInvalid(temp.startByte, endByte), // relevant part of instructions
                    CccmLib.extractSubset(packets_, temp.lastFrgmtCnt +1, numFrgmt),
                    responses,
                    successFlags
                )
            );

SOLVED : The Concrete team solved the issue. They incremented to one the temp.lastFrgmtCnt variable in order to select the proper value for executing the external call.

It has been identified that there is no restriction on the size of the packets being formed, allowing for the encoding of excessively large packets. This lack of limitation can lead to unexpected behaviors and potential Denial of Service (DoS) attacks when attempting to iterate over packets_.length before making external calls.

function multipleReceiveOneBatchPseudoInternal(bytes[] calldata packets_)
        external
        OnlyPseudoInternalCalls
        returns (bytes[] memory responses)
    {
        bool callSuccess;
        responses = new bytes[](packets_.length - 1);
        for (uint256 i = 1; i < packets_.length; i++) {
            (callSuccess, responses[i - 1]) = address(bytes20(packets_[i][:20])).call(packets_[i][20:]);
            if (!callSuccess) {
                revert EndpointErrors.OneCallInBatchedCallReverted(uint8(i - 1));
            }
        }
    }

Potential Impact:

• Unexpected Behaviors: The function may exhibit unanticipated actions due to the lack of packet size control.

• Denial of Service (DoS): Iterating over large packet arrays can lead to significant delays or failures, impacting the system's reliability and availability.

The following Foundry test was used in order to prove the aforementioned issue:

function test_cccmMultiReceiveWithOutInstructions_TooMany() public {
        uint256 C = 25;
        // generate two calldata packets. One is for retrieving the B value an
        // the second is for addToA some value
        bytes[] memory packets = new bytes[](10000);
        // ["", packet1, packet2]
        packets[0] = "";
        packets[1] = abi.encodePacked(address(mockCallContract), MockCallContract.getB.selector);
        packets[2] = abi.encodePacked(address(mockCallContract), MockCallContract.addToA.selector, C);

        for (uint256 index = 3; index < 10000; index++) {
            packets[index] = abi.encodePacked(address(mockCallContract), MockCallContract.addToA.selector, C);
        }
        // expected responses
        bytes[] memory responses = new bytes[](2);
        responses[0] = abi.encode(B);
        responses[1] = abi.encode(A + C);
        // no response template
        bytes memory responseTemplate = "";
        // send the packets without instructions and expect event to be emitted
        vm.startPrank(executor);
        uint224 nonce = uint224(multiMessaging.getNonce());
        bytes32 packetId = bytes32((uint256(nonce) << 32) | uint256(targetEid));
        // check if the event was emitted
        vm.expectEmit(true, true, true, true, address(multiMessaging));
        emit EndpointMultiMessageEvents.MultiResponse(packetId, keccak256(abi.encode(packets)), 3, responses);
        multiMessaging.cccmMultiReceive(packetId, eid, packets, responseTemplate);
        vm.stopPrank();
    }

Evidence: Allowed executed a packet of 10000 size, and the huge event emitting with all responses array.

Consider implementing a restriction on the maximum allowable size of packets to prevent encoding excessively large packets. Moreover, validate packet sizes before processing to ensure they fall within acceptable limits.

SOLVED: The suggested mitigation was applied to the following functions:

• MultiMessaging::cccmMultiSend()

• MultiMessaging::cccmMultiReceive()

• SingleMessaging::cccmSingleSend()

• SingleMessaging::cccmSingleReceive()

In the MultiMessaging and SingleMessaging contract, the function "Receive" involves an external call to the target packets.

While the contract's current implementation presents no obvious vulnerability due to the fact that the external call is made by the executor (Role guard), it is advisable to include the nonReentrant modifier for enhanced security and to prevent unexpected behavior. This is a safer practice and can prevent potential issues in future updates or unforeseen scenarios.

Add a nonReentrant type modifier to the function to further enhance security.

SOLVED: A modified reentrancy guard was added, which prevents smart contracts from reentering.

   modifier nonReentrantForContracts() {
         if (msg.sender == tx.origin) {
             _;
         } else {
             _nonReentrantBefore();
             _;
             _nonReentrantAfter();
         }
     }

Adding events to the setEid and setNativeTokenRecipient functions would provide better traceability of state changes.

Consider defining and emit events for these functions.

event EidSet(uint32 indexed newEid);
event NativeTokenRecipientSet(address indexed newRecipient);

function setEid(uint32 eid_) external override(IEid) onlyRole(EndpointRoles.SETTER) {
    if (eid_ == 0) revert EndpointErrors.InvalidTargetEid(eid_);
    EID = eid_;
    emit EidSet(eid_);
}

function setNativeTokenRecipient(address payable recipient_) external override(ISendNativeToken) onlyRole(EndpointRoles.MANAGER) {
    if (recipient_ == address(0)) revert EndpointErrors.InvalidNativeTokenRecipientAddress();
    _nativeTokenRecipient = recipient_;
    emit NativeTokenRecipientSet(recipient_);
}

SOLVED: The suggested mitigation was implemented by the Concrete team.