When `src==dest`, `redeemTicket()` will become unusable.

[hats-bug-reporter[bot]]

Github username: @9olidity Submission hash (on-chain): 0x7ec10f183a2c176578f059a0e81592ca36c1e6f1c377ec067808fcc5f6ee0438 Severity: medium

Description: Description\ When src==dest, redeemTicket() will become unusable.

Attack Scenario\

When executing _redeemTicketInternal, the relationship between self and source will be checked, but there is no restriction in the code here that self is equal to source. When self == source is equal, the first execution of _redeemTicketInternal can be executed normally, but the second When calling _redeemTicketInternal , an InvalidAggregatedTicketInterval error occurs and redeemTicket() cannot execute normally.

Attachments

1. Proof of Concept (PoC) File

forge test --match-test test_sameredeemTicket -vvvv

    function test_sameredeemTicket(
        uint256 privKeyA,
        uint256 privKeyB,
        address safeContract,
        uint256 porSecret,
        uint96 channelAmount,
        uint96 amount,
        uint48 maxTicketIndex,
        uint32 indexOffset,
        uint24 epoch,
        uint48 channelTicketIndex
    )
        public
    {
        porSecret = bound(porSecret, 1, HoprCrypto.SECP256K1_FIELD_ORDER - 1);
        privKeyA = bound(privKeyA, 1, HoprCrypto.SECP256K1_FIELD_ORDER - 1);
        privKeyB = bound(privKeyB, 1, HoprCrypto.SECP256K1_FIELD_ORDER - 1);
        vm.assume(privKeyA != privKeyB);
        amount = uint96(bound(amount, MIN_USED_BALANCE, MAX_USED_BALANCE));
        channelAmount = uint96(bound(channelAmount, amount, MAX_USED_BALANCE));
        indexOffset = uint32(bound(indexOffset, 1, type(uint32).max));
        channelTicketIndex = uint48(bound(channelTicketIndex, 0, type(uint48).max - indexOffset - 1));
        maxTicketIndex = uint48(bound(maxTicketIndex, channelTicketIndex + 1, type(uint48).max - indexOffset));

        address src = vm.addr(privKeyA);
        address dest = vm.addr(privKeyB);
        vm.assume(safeContract != address(0) && safeContract != src && safeContract != dest);

        // _helperNoSafeSetMock(dest);
        _helperOnlySafeMock(dest,dest);
        // _helperOnlySafeMock(dest,src);
        _helperTokenTransferMock(dest, amount);

        RedeemTicketArgBuilder memory args = RedeemTicketArgBuilder(
            privKeyB,
            privKeyB,
            hoprChannels.domainSeparator(),
            dest,
            dest,
            amount,
            maxTicketIndex,
            indexOffset,
            epoch,
            HoprChannels.WinProb.unwrap(WIN_PROB_100),
            porSecret
        );

        hoprChannels._storeChannel(
            dest, dest, channelAmount, channelTicketIndex, 0, epoch, HoprChannels.ChannelStatus.OPEN
        );

        (HoprChannels.RedeemableTicket memory redeemable, HoprCrypto.VRFParameters memory vrf) =
            CryptoUtils.getRedeemableTicket(args);

        hoprNodeSafeRegistry.nodeToSafe(dest);
        vm.prank(dest);
        // hoprChannels.redeemTicket(redeemable, vrf);
        hoprChannels.redeemTicketSafe(dest, redeemable, vrf);
        vm.prank(dest);
        vm.expectRevert(HoprChannels.InvalidAggregatedTicketInterval.selector);
        hoprChannels.redeemTicketSafe(dest, redeemable, vrf);
    }

2. Revised Code File (Optional)

   
   function _redeemTicketInternal(
       address self,
       RedeemableTicket calldata redeemable,
       HoprCrypto.VRFParameters calldata params
   )
       internal
       validateBalance(redeemable.data.amount)
       HoprCrypto.isFieldElement(redeemable.porSecret)
   {
       Channel storage spendingChannel = channels[redeemable.data.channelId];
   
       if (spendingChannel.status != ChannelStatus.OPEN && spendingChannel.status != ChannelStatus.PENDING_TO_CLOSE) {
           revert WrongChannelState({ reason: "spending channel must be OPEN or PENDING_TO_CLOSE" });
       }
   
       if (ChannelEpoch.unwrap(spendingChannel.epoch) != ChannelEpoch.unwrap(redeemable.data.epoch)) {
           revert WrongChannelState({ reason: "channel epoch must match" });
       }
   
       // Aggregatable Tickets - validity interval:
       // A ticket has a base index and an offset. The offset must be > 0,
       // while the base index must be >= the currently set ticket index in the
       // channel.
       uint48 baseIndex = TicketIndex.unwrap(redeemable.data.ticketIndex);
       uint32 baseIndexOffset = TicketIndexOffset.unwrap(redeemable.data.indexOffset);
       uint48 currentIndex = TicketIndex.unwrap(spendingChannel.ticketIndex);
       if (baseIndexOffset 

[fonstack]

@9olidity please complete your submission with a comment here. The submission was not 100% submitted.

[9olidity]

Recommendation

    function _redeemTicketInternal( 
        address self,
        RedeemableTicket calldata redeemable,
        HoprCrypto.VRFParameters calldata params
    )
        internal
        validateBalance(redeemable.data.amount)
        HoprCrypto.isFieldElement(redeemable.porSecret)
    {
        Channel storage spendingChannel = channels[redeemable.data.channelId];

        if (spendingChannel.status != ChannelStatus.OPEN && spendingChannel.status != ChannelStatus.PENDING_TO_CLOSE) {
            revert WrongChannelState({ reason: "spending channel must be OPEN or PENDING_TO_CLOSE" });
        }

        if (ChannelEpoch.unwrap(spendingChannel.epoch) != ChannelEpoch.unwrap(redeemable.data.epoch)) {
            revert WrongChannelState({ reason: "channel epoch must match" });
        }

        // Aggregatable Tickets - validity interval:
        // A ticket has a base index and an offset. The offset must be > 0,
        // while the base index must be >= the currently set ticket index in the
        // channel.
        uint48 baseIndex = TicketIndex.unwrap(redeemable.data.ticketIndex);
        uint32 baseIndexOffset = TicketIndexOffset.unwrap(redeemable.data.indexOffset);
        uint48 currentIndex = TicketIndex.unwrap(spendingChannel.ticketIndex);
        if (baseIndexOffset < 1 || baseIndex < currentIndex) {
            revert InvalidAggregatedTicketInterval();
        }

        if (Balance.unwrap(spendingChannel.balance) < Balance.unwrap(redeemable.data.amount)) {
            revert InsufficientChannelBalance();
        }

        // Deviates from EIP712 due to computed property and non-standard struct property encoding
        bytes32 ticketHash = _getTicketHash(redeemable);

        if (!_isWinningTicket(ticketHash, redeemable, params)) {
            revert TicketIsNotAWin();
        }

        HoprCrypto.VRFPayload memory payload =
            HoprCrypto.VRFPayload(ticketHash, self, abi.encodePacked(domainSeparator));

        if (!vrfVerify(params, payload)) {
            revert InvalidVRFProof();
        }

        address source = ECDSA.recover(ticketHash, redeemable.signature.r, redeemable.signature.vs);
+       require(source != self);
        if (_getChannelId(source, self) != redeemable.data.channelId) {
            revert InvalidTicketSignature();
        }

        spendingChannel.ticketIndex = TicketIndex.wrap(baseIndex + baseIndexOffset);
        spendingChannel.balance =
            Balance.wrap(Balance.unwrap(spendingChannel.balance) - Balance.unwrap(redeemable.data.amount));
        indexEvent(
            abi.encodePacked(ChannelBalanceDecreased.selector, redeemable.data.channelId, spendingChannel.balance)
        );
        emit ChannelBalanceDecreased(redeemable.data.channelId, spendingChannel.balance);

        bytes32 outgoingChannelId = _getChannelId(self, source);
        Channel storage earningChannel = channels[outgoingChannelId];

        // Informs about new ticketIndex
        indexEvent(abi.encodePacked(TicketRedeemed.selector, redeemable.data.channelId, spendingChannel.ticketIndex));
        emit TicketRedeemed(redeemable.data.channelId, spendingChannel.ticketIndex);

        if (earningChannel.status == ChannelStatus.CLOSED) {
            // The other channel does not exist, so we need to transfer funds directly
            if (token.transfer(msg.sender, Balance.unwrap(redeemable.data.amount)) != true) {
                revert TokenTransferFailed();
            }
        } else {
            // this CAN produce channels with more stake than MAX_USED_AMOUNT - which does not lead
            // to overflows since total supply < type(uin96).max
            earningChannel.balance =
                Balance.wrap(Balance.unwrap(earningChannel.balance) + Balance.unwrap(redeemable.data.amount));
            indexEvent(abi.encodePacked(ChannelBalanceIncreased.selector, outgoingChannelId, earningChannel.balance));
            emit ChannelBalanceIncreased(outgoingChannelId, earningChannel.balance);
        }
    }

[QYuQianchen]

It's forbidden to open a channel where src is equal to dest. Therefore it's expected that redeemTicket() is not allowed in this case.

[9olidity]

@QYuQianchen There is no such restriction in the code, so I think this vulnerability is valid

[QYuQianchen]

There is. See usage and definition