Delegations are not added in allDelegations array within the delegate() function.

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x67a1843d023635cfbd956e01ed33eb2433a628427d6b8811ab73133332f9802b
Severity: low

Description:
Description
In the stROSEMinter contract delegations(StakingAddress) are added in the allDelegations array only when takeReceiptDelegate() function is performed which should not be the expected behaviour. Delegations should be added within delegate() function instead and removed from takeReceiptDelegate() function. In this way the list of all delegates(allDelegations) will be accurate right after performing the function delegate().

Impact
When getAllDelegations()function is performed before takeReceiptDelegate() the list of all delegations will not be full and accurate. This will cause issues via missing info.

Attachments

1. Proof of Concept (PoC) File

function takeReceiptDelegate(uint64 receiptId) public onlyOwner returns (uint128 shares) {
        DelegationReceipt storage receipt = delegationReceipts[receiptId];
        require(block.number > receipt.blockNumber, "ReceiptNotReady");
        require(receipt.exists, "ReceiptNotExists");
        require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
        shares = Subcall.consensusTakeReceiptDelegate(receiptId);
        Delegation storage d = delegations[receipt.to];

        // update receipt with the necessary info
        receipt.shares = shares;
        receipt.receiptTaken = true;
        receipt.receiptTakenBlockNumber = block.number;

        // update delegation amount and shares
        d.shares += shares;
        
    @>        _addDelegation(receipt.to);
        emit TakeReceiptDelegate(receiptId);
    }

2. Revised Code File

The _addDelegation() function should be moved whithin delegate() function. This will lead to proper adding of delegates in allDelegates array.

function delegate(StakingAddress to, uint128 amount) public onlyOwner returns (uint64) {
        require(amount < type(uint128).max, ">MaxUint128");
        require(amount > 0, "ZeroDelegate");
        uint64 receiptId = nextReceiptId++;
        Subcall.consensusDelegate(to, amount, receiptId);
        delegationReceipts[receiptId] = DelegationReceipt({
            exists: true,
            to: to,
            blockNumber: block.number,
            receiptTaken: false,
            receiptTakenBlockNumber: 0,
            shares: 0,
            amount: amount
        });

+     _addDelegation(to);

        emit Delegate(to, amount, receiptId);
        return receiptId;
    }

function takeReceiptDelegate(uint64 receiptId) public onlyOwner returns (uint128 shares) {
        DelegationReceipt storage receipt = delegationReceipts[receiptId];
        require(block.number > receipt.blockNumber, "ReceiptNotReady");
        require(receipt.exists, "ReceiptNotExists");
        require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
        shares = Subcall.consensusTakeReceiptDelegate(receiptId);
        Delegation storage d = delegations[receipt.to];

        // update receipt with the necessary info
        receipt.shares = shares;
        receipt.receiptTaken = true;
        receipt.receiptTakenBlockNumber = block.number;

        // update delegation amount and shares
        d.shares += shares;
        
-        _addDelegation(receipt.to);
        emit TakeReceiptDelegate(receiptId);
    }

[ilzheev]

delegate() and takeReceiptDelegate() are always executed within 1 block – i.e. at the same time.

[0xqaswed]

@ilzheev thanks for your comment.

According to the comment above the takeReceiptDelegate() function the delegate() and takeReceiptDelegate() will not be included in the same block. Also, there is a check where the current block number must be > than the receipt.blocknumber

/**
     * Retrieve the number of shares received in return for delegation.
     *
     * The receipt will only be available after the delegate transaction has
     * been included in a block. It is necessary to wait for the message to
     * reach the consensus layer and be processed to determine the number of
     * shares.
     *
     * @param receiptId Receipt ID previously emitted/returned by `delegate`.
     */

    function takeReceiptDelegate(uint64 receiptId) public onlyOwner returns (uint128 shares) {
        DelegationReceipt storage receipt = delegationReceipts[receiptId];
  @>      require(block.number > receipt.blockNumber, "ReceiptNotReady");
        require(receipt.exists, "ReceiptNotExists");
        require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
        shares = Subcall.consensusTakeReceiptDelegate(receiptId);
        Delegation storage d = delegations[receipt.to];

        // update receipt with the necessary info
        receipt.shares = shares;
        receipt.receiptTaken = true;
        receipt.receiptTakenBlockNumber = block.number;

        // update delegation amount and shares
        d.shares += shares;
        
        _addDelegation(receipt.to);
        emit TakeReceiptDelegate(receiptId);
    }

[ilzheev]

According to the comment above the takeReceiptDelegate() function the delegate() and takeReceiptDelegate() will not be included in the same block.

Yeah, I meant both txs are submitted with 1 block difference (i.e. one by one), there is 0 chance that second tx won't be completed

[0xqaswed]

@ilzheev , sorry for the late response.

The source code doesn't indicate anywhere that takeReceiptDelegate() will be executed exactly one block(6 sec) after delegate() - they are 2 separate functions independent from each other. Even one block time(6 sec) is enough for getAllDelegations() to respond with inaccurate info. Also, saying there's "zero chance the second transaction won't complete" is overly optimistic, everything can happen - connection issue, node goes down, etc. Regardless of the submission validity, please move _addDelegation() in the delegate(), this is the correct place that the allDelegations array should be updated otherwise, this can lead to a lack of updated info.