4b - Incorrect casting in `BaseCCIPContract::_packCCIPContract()`

[sherlock-admin4]

4b

Medium

Incorrect casting in BaseCCIPContract::_packCCIPContract()

Summary

The style of casting the chainSelector is incorrect hence it will evaluate to zero and not form part of the bytes returned.

Root Cause

In BaseCCIPContract::_packCCIPContract(), the chainSelector() was casted to uint256 after the bitwise operation which is not supposed to be so

Internal pre-conditions

whenever the function params are set it will occur

External pre-conditions

There are no preconditions, anytime it is called by another function it will occur.

Attack Path

This is an internal casting error so whenever another function calls BaseCCIPContract::_packCCIPContract() it will occur.

Impact

The _packCCIPContract() returns an incorrect value since the chainSelector will not be part of the encoded bytes.

PoC

Code with the casting error

    function _packCCIPContract(address contractAddress, uint64 chainSelector) internal pure returns(bytes32) { return bytes32(
            uint256(uint160(contractAddress)) | uint256(chainSelector << 160));
    }

these are the test results after supplying it with random values

[PASS] test() (gas: 5678)
Traces:
  [5678] returnTest::test_packCCIPContract()
    ├─ [427] BitwiseOperations::_packCCIPContract(returnTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 43114 [4.311e4]) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496
    └─ ← [Stop] 

Suite result: ok. 1 passed;

we can observe that the test passed but only the address has been encoded

code with the right casting style

    function CCIP(address contractAddress, uint64 chainSelector) public pure returns(bytes32) {
        return bytes32(
            uint256(uint160(contractAddress)) |
            (uint256(chainSelector) << 160)
        );
    }

these are the test results

[PASS] testCCIP() (gas: 5705)
Traces:
  [5705] returnTest::test_packCCIPContract()
    ├─ [454] BitwiseOperations::_packCCIPContract(returnTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 43114 [4.311e4]) [staticcall]
    │   └─ ← [Return] 0x00000000000000000000a86a7fa9385be102ac3eac297483dd6233d62b3e1496
    └─ ← [Stop] 

Suite result: ok. 1 passed;

from this test result we can observe that the chainSelector has been encoded to the bytes32 returned and not only the address

Mitigation

Instead of casting after the bitwise operation, cast before the operation

    function _packCCIPContract(address contractAddress, uint64 chainSelector) internal pure returns(bytes32) {
        return bytes32(
-            uint256(uint160(contractAddress)) | uint256(chainSelector << 160));
+            uint256(uint160(contractAddress)) | uint256(chainSelector) << 160);
    }

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Winnables/public-contracts/pull/7

[neko-nyaa]

Escalate. No impact.

The casting is incorrect, yes. But the result is just that the CCIP contract info includes the address without the chain selector. Per the README, the contracts are only going to be deployed on two chains (with no contract being deployed on more than one chain), the chain selector is redundant. Message will still be sent and processed normally.

[sherlock-admin3]

Escalate. No impact.

The casting is incorrect, yes. But the result is just that the CCIP contract info includes the address without the chain selector. Per the README, the contracts are only going to be deployed on two chains (with no contract being deployed on more than one chain), the chain selector is redundant. Message will still be sent and processed normally.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[DemoreXTess]

@neko-nyaa I disagree with the escalation. Contracts from different chains could send false information to both contracts, allowing an attacker to win every raffle in the system. Therefore, this should be considered a valid issue.

[rbserver]

Hi, I also disagree with the escalation. Some duplicates of this report, such as #415, #458, #464, #588, and #596, describe the high/medium impact of this issue.

[mystery0x]

The very fact that incorrect bitwise shifting that renders packed CCIP whitelist to ignore the chainSelector should make this a valid medium.

[WangSecurity]

I'm not sure I understand the impact of this report (I see there are other reports mentioned that have higher impact), but what about this?

The _packCCIPContract() returns an incorrect value since the chainSelector will not be part of the encoded bytes

How is that severe in the context of the protocol and how it prevents the protocol from functioning correctly?

[DemoreXTess]

@WangSecurity

packCCIPContract() is used for whitelisting a specific contract ( it will be in another chain ) for message receiving. It's packing the chain id and contract address with bitwise operation in order to check that value in later.

Message sending and receiving are crucial features of the system. For instance, a raffle is finalized and ticket manager propagate the winner to the prize manager contract. Prize manager will check whether the message comes from correct chain and address. In this context, both of the contracts can accept message from unapproved chains. In conclusion, attacker's contract can send false information to prize manager to win raffles from unapproved chain.

[neko-nyaa]

The chain is not validated but the address is still validated. For the false information to be consumed, the attacker's contract on the wrong chain has to have the exact same address as with the true contract on the correct chain.

It is impossible to deploy a contract on the exact chosen address on a different chain unless the protocol uses a factory pattern (which is not the case based on available testing/deployment scripts), or you own the deployer's address (which is equivalent to a private key compromise).

[WangSecurity]

Based on the above comment I agree this issue is indeed low impact. But, of course, anyone can dispute this and clarify. Planning to accept the escalation and invalidate this finding.

[Brivan-26]

@WangSecurity Given the fact that chainId is not encoded and is ignored, transactions with contracts on unwhitlisted chains will still pass.

[WangSecurity]

@Brivan-26 as I understand based on this comment, it's impossible for this to happen. But, please correct me if it's wrong. Until then, the decision remains the same, planning to accept the escalation and invalidate the issue.

[0xDenzi]

@WangSecurity It is considered astronomically unlikely to have the same address (not impossible as the comment says). So the likelihood can be considered very low with the impact as shown in the reports to be high.

Further more, as auditors we do not go through the deployment/testing scripts as they can change pre/post audits to the liking of the devs. Based on the above reason we also do not consider front-running initializers to be medium and they are always given low/info severity because that issue is related to how the contracts are deployed so we can not take the deployment scripts as a good foundation to judge the issue.

[WangSecurity]

As I understand, it's indeed impossible in this case.

To deploy the contract with the same address on different chains, it must be deployed from the same address (i.e. EOA), must have the same address nonce and contracts must have the same bytecode. Hence, the issue described in the report indeed cannot happen. The decision remains, reject the escalation and leave the issue as it is.

[AtanasDimulski]

Hey @WangSecurity the escalation is to invalidate the issue, from the first part it looks like you think it should be invalid, and from the second you are saying you will leave the issue as is, which one is it?

[WangSecurity]

Yes, you're correct, I made a typo, planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• neko-nyaa: accepted

[ulasacikel]

must have the same address nonce and contracts must have the same bytecode.

@WangSecurity this is not true. For create opcode the nonce and the address being the same is enough.