end time is not validated in push payments and it can be set in past

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x03ed2b20bd5a77672579d30426d5c2be0b74e333d4053ca034d5e2b52199f2c9 Severity: low

Description: Description\

LM_PC_PaymentRouter_v1.sol is used to push payments directly to the Payment Processor. This contracts has pushPayment() function which process single payment and pushPaymentBatched() function process batch payments.

    function pushPayment(
        address recipient,
        address paymentToken,
        uint amount,
        uint start,
        uint cliff,
        uint end     @audit // end timestamp is not validated
    ) public onlyModuleRole(PAYMENT_PUSHER_ROLE) {
        _addPaymentOrder(
            PaymentOrder(
                recipient,
                paymentToken,
                amount,
                start == 0 ? block.timestamp : start,
                cliff,
                end        @audit // end timestamp could be in past

            )
        );

        // call PaymentProcessor
        __Module_orchestrator.paymentProcessor().processPayments(
            IERC20PaymentClientBase_v1(address(this))
        );
    }

    function pushPaymentBatched(
        uint8 numOfOrders,
        address[] calldata recipients,
        address[] calldata paymentTokens,
        uint[] calldata amounts,
        uint start,
        uint cliff,
        uint end               @audit // end timestamp is not validated
    ) public onlyModuleRole(PAYMENT_PUSHER_ROLE) {    
        // Validate all arrays have the same length
        if (
            recipients.length != numOfOrders
                || paymentTokens.length != numOfOrders
                || amounts.length != numOfOrders
        ) {
            revert Module__ERC20PaymentClientBase__ArrayLengthMismatch();
        }

        // Loop through the arrays and add Payments
        for (uint8 i = 0; i < numOfOrders; i++) {
            _addPaymentOrder(
                PaymentOrder(
                    recipients[i],
                    paymentTokens[i],
                    amounts[i],
                    start == 0 ? block.timestamp : start,
                    cliff,
                    end       @audit // end timestamp could be in past
                )
            );
        }

The issue is that, the end timestmp which is used as the timestamps at which the payments should be fulfilled is not validated. It means that, it can be set to 0 or the timestamp can be set which is already passed i.e in past time. If this happens, the end timestamp wont be able to fulfill the payment within time.

end time should be more or equal to block.timestamp and it should be in future and must be validated at the start of function.

Recommendation to fix\ Consider validating the end timestamp in both function:

    function pushPayment(
        address recipient,
        address paymentToken,
        uint amount,
        uint start,
        uint cliff,
        uint end
    ) public onlyModuleRole(PAYMENT_PUSHER_ROLE) {
+      require(end >= block.timestamp, "invalid timestamp");
        _addPaymentOrder(
            PaymentOrder(
                recipient,
                paymentToken,
                amount,
                start == 0 ? block.timestamp : start,
                cliff,
                end
            )
        );

        // call PaymentProcessor
        __Module_orchestrator.paymentProcessor().processPayments(
            IERC20PaymentClientBase_v1(address(this))
        );
    }

    /// @inheritdoc ILM_PC_PaymentRouter_v1
    function pushPaymentBatched(
        uint8 numOfOrders,
        address[] calldata recipients,
        address[] calldata paymentTokens,
        uint[] calldata amounts,
        uint start,
        uint cliff,
        uint end
    ) public onlyModuleRole(PAYMENT_PUSHER_ROLE) {    
+      require(end >= block.timestamp, "invalid timestamp");
        // Validate all arrays have the same length
        if (
            recipients.length != numOfOrders
                || paymentTokens.length != numOfOrders
                || amounts.length != numOfOrders
        ) {
            revert Module__ERC20PaymentClientBase__ArrayLengthMismatch();
        }

        // Loop through the arrays and add Payments
        for (uint8 i = 0; i < numOfOrders; i++) {
            _addPaymentOrder(
                PaymentOrder(
                    recipients[i],
                    paymentTokens[i],
                    amounts[i],
                    start == 0 ? block.timestamp : start,
                    cliff,
                    end
                )
            );
        }

[FHieser]

the end timestamp does only matter in the streaming payment processor to compare it to the start time. In combination with the streamingpaymentProcessor it might also be advantageous to keep the end timestamp in the past I would say this is invalid