denzi_ - Incorrect Bit Shifting in `BaseCCIPContract::_packCCIPContract()` Function

[sherlock-admin3]

denzi_

Medium

Incorrect Bit Shifting in BaseCCIPContract::_packCCIPContract() Function

Summary

An issue has been identified in the _packCCIPContract() function where incorrect bit shifting of the chainSelector results in its value being effectively set to zero. This leads to potential security risks where trusted contract validations may not perform as intended, allowing unauthorized cross-chain interactions.

Vulnerability Detail

The _packCCIPContract() function is designed to pack an address and a chain selector into a single bytes32 variable. However, due to incorrect handling of type casting and bit shifting, the chain selector value is being lost. This function takes the chainSelector as uint64 and then shifts it 160 places left before converting it to uint256. Due to this, the bits representing the chain selector move out of the 256-bit range and the value is set to zero in the resulting bytes32.

    /// @dev Linked CCIP contracts
    /// The mapping key is a packed bytes32 with the following bit mapping
    /// [0..159]    address sourceContract
    /// [160..223]  uint64  sourceChainSelector
    mapping(bytes32 => bool) internal _ccipContracts;

This mapping holds the enabled _ccipContracts which are made up of two components sourceContract and sourceChainSelector.

Once the contracts are set up, this mapping will now hold the correct address but 0 as its sourceChainSelector.

The mapping is used to validate the messages being received in _ccipReceive() functions present in two contracts WinnablesTicketManager and WinnablesPrizeManager.

Both functions have a check on the sender's address and chainSelector

        (address _senderAddress) = abi.decode(message.sender, (address));
        bytes32 counterpart = _packCCIPContract(_senderAddress, message.sourceChainSelector);
        if (!_ccipContracts[counterpart]) revert UnauthorizedCCIPSender();

The improper bit shifting creates a problem here because the counterpart returned will have no chainSelector in it, allowing the same address from other chains to bypass this check and execute their message.

1. WinnablesPrizeManager::_ccipReceive() allows raffles to be cancelled or a winner to be drawn according to the data from the message.
2. WinnablesTicketManager::_ccpReceive() allows raffles to be created on any id, removing the NONE status for the corresponding id.

POC

Paste the following code into remix and provide any address and chainSelector e.g

1. address = 0x03C6FcED478cBbC9a4FAB34eF9f40767739D1Ff7
2. chainSelector = 33311

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

The output is the following and it only contains the address bytes32: 0x00000000000000000000000003c6fced478cbbc9a4fab34ef9f40767739d1ff7

Impact

Although the likelihood of having the same address as the authorized address on a different chain is low but the impact which can be caused by this issue is dangerous as it can allow a malicious user to manipulate the status and draw winners of raffles.

Code Snippet

BaseCCIPContract.sol WinnablesTicketManager.sol WinnablesPrizeManager.sol

Tool used

Manual Review, Remix

Recommendation

Change the code to

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

Duplicate of #72