Validate the new HashChainCapacitor packet length instead of the old state value

[chenshj73]

Hi Socket team, I noticed a small but concrete validation bug in HashChainCapacitor while reviewing the packet/message lifecycle code.

The constructor and updateMaxPacketLength intend to cap packet length at MAX_LEN, but both checks read the current state variable maxPacketLength instead of the new input maxPacketLength_. This lets oversized packet lengths be installed.

Commit reviewed: 4df06c9eaf3fa1b65e51a6dd278dd4545814f523

Relevant code

// contracts/capacitors/HashChainCapacitor.sol:50-63
    /**
     * @dev Initializes the contract with the specified socket address.
     * @param socket_ The address of the socket contract.
     * @param owner_ The address of the owner of the capacitor contract.
     * @param maxPacketLength_ The max Packet Length of the capacitor contract.
     */
    constructor(
        address socket_,
        address owner_,
        uint256 maxPacketLength_
    ) BaseCapacitor(socket_, owner_) {
        if (maxPacketLength > MAX_LEN) revert InvalidPacketLength();
        maxPacketLength = maxPacketLength_;
    }

At construction time maxPacketLength is still the default state value (0), so the check never rejects an oversized maxPacketLength_.

The update path has the same issue:

// contracts/capacitors/HashChainCapacitor.sol:65-99
    /**
     * @notice Update packet length of the hash chain capacitor.
     * @notice Only owner can call this function
     * @dev The function will update the packet length of the hash chain capacitor, and also create any packets
     * if the new packet length is less than the current packet length.
     * @param maxPacketLength_ The new nax packet length of the hash chain.
     */
    function updateMaxPacketLength(
        uint256 maxPacketLength_
    ) external onlyOwner {
        if (maxPacketLength > MAX_LEN) revert InvalidPacketLength();
        if (maxPacketLength_ < maxPacketLength) {
            uint64 lastPackedMsgIndex = messagePacked;
            uint64 packetCount = _nextPacketCount;
            uint64 packets = (nextMessageCount - lastPackedMsgIndex) %
                uint64(maxPacketLength_);

            _nextPacketCount += packets;

            for (uint64 index = 0; index < packets; ) {
                uint64 packetEndAt = lastPackedMsgIndex +
                    uint64(maxPacketLength_);

                _roots[packetCount + index] = messageRoots[packetEndAt];
                lastPackedMsgIndex = packetEndAt;
                unchecked {
                    ++index;
                }
            }
            messagePacked = lastPackedMsgIndex;
        }

        maxPacketLength = maxPacketLength_;
        emit MaxPacketLengthSet(maxPacketLength_);
    }

If the current length is below the cap, updateMaxPacketLength(11) will pass even though MAX_LEN is 10; only a later call would start reverting after the invalid state has already been stored.

That state is then used as the packet lifecycle bound in sealing:

// contracts/capacitors/HashChainCapacitor.sol:140-165
    function sealPacket(
        uint256 batchSize
    ) external override onlySocket returns (bytes32 root, uint64 packetCount) {
        uint256 messageCount = nextMessageCount;

        // revert if batch size exceeds max length
        if (batchSize > maxPacketLength) revert InvalidBatchSize();

        packetCount = _nextSealCount++;
        if (_roots[packetCount] == bytes32(0)) {
            // last message count included in this packet
            uint64 lastMessageCount = messagePacked + uint64(batchSize);

            // if no message found or total message count is less than expected length
            if (messageCount <= lastMessageCount)
                revert InsufficientMessageLength();

            _createPacket(
                packetCount,
                lastMessageCount,
                messageRoots[lastMessageCount]
            );
        }

        root = _roots[packetCount];
    }

I also noticed the current factory only deploys SingleCapacitor, despite the comment mentioning future hash-chain/batching support:

// contracts/CapacitorFactory.sol:42-70
    /**
     * @notice Creates a new capacitor and decapacitor pair based on the given type.
     * @dev It sets the CapacitorFactory owner as owner of new Capacitor and Decapacitor
     * @param capacitorType_ The type of capacitor to be created. Can be SINGLE_CAPACITOR or HASH_CHAIN_CAPACITOR.
     * @dev siblingChainSlug_ sibling chain slug can be used for chain specific capacitors, useful while expanding to non-EVM chains.
     * @param maxPacketLength_ is not being used with single capacitor system, will be useful with batching.
     */
    function deploy(
        uint256 capacitorType_,
        uint32 /** siblingChainSlug_ */,
        uint256 maxPacketLength_
    ) external override returns (ICapacitor, IDecapacitor) {
        if (
            maxPacketLength_ < minAllowedPacketLength ||
            maxPacketLength_ > maxAllowedPacketLength
        ) revert PacketLengthNotAllowed();

        // fetch the capacitor factory owner
        address owner = this.owner();

        if (capacitorType_ == SINGLE_CAPACITOR) {
            return (
                // msg.sender is socket address
                new SingleCapacitor(msg.sender, owner),
                new SingleDecapacitor(owner)
            );
        }
        revert InvalidCapacitorType();
    }

So this may not affect the current default deployment path, but the standalone HashChainCapacitor contract and its tests are present. If/when batching is enabled again, this can produce an invalid packet-size lifecycle state.

Suggested fix

Use the input value in both checks:

if (maxPacketLength_ > MAX_LEN) revert InvalidPacketLength();

It may also be worth rejecting zero in the capacitor itself, not only in the factory, so direct deployments cannot create a capacitor that never auto-creates packets.