Incorrect Index Emission in _updateRingKey Function

[hats-bug-reporter]

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

Description:
Description\

function _updateRingKey(bytes32 _entropy) internal {
    bytes32 newKey = bytes32(Sapphire.randomBytes(32, abi.encodePacked(_entropy)));

    uint newIndex = _ringKeys.length; // This is calculated before pushing the new key
    _ringKeys.push(newKey);

    _lastRingKeyUpdate = block.timestamp;

    emit ActualRingKeyRenewed(newIndex); // Emits the old index
}

The _updateRingKey function is responsible for generating a new ring key, adding it to the _ringKeys array, and emitting an event to log the update. However, there is a logical error in the current implementation that causes the event to emit an incorrect index.

The newIndex is calculated before the new key is pushed to the _ringKeys array. As a result, newIndex represents the length of the array before the new key is added, which is incorrect.

Attack Scenario\

1. Call the _updateRingKey function with any valid _entropy value.
2. Observe the emitted ActualRingKeyRenewed event.
   Expected Behavior:
   The ActualRingKeyRenewed event should emit the index of the newly added ring key.
   Actual Behavior:
   The ActualRingKeyRenewed event emits the index of the previous key instead of the new one.

Attachments

1. Proof of Concept (PoC) File

• Index Calculation: uint newIndex = _ringKeys.length; calculates the index based on the current length of the _ringKeys array.
• Pushing the New Key: _ringKeys.push(newKey); adds the new key to the array, increasing its length by one.
• Emitting the Event: emit ActualRingKeyRenewed(newIndex); emits the index that was calculated before the new key was added, which is off by one.

Impact:
This issue can lead to incorrect tracking of ring key updates, causing potential confusion and errors in any off-chain systems or logs that rely on the ActualRingKeyRenewed event for accurate information.

2. Revised Code File (Optional)

• To ensure the correct index is emitted, calculate the newIndex after pushing the new key to the _ringKeys array. Here is the corrected implementation:

function _updateRingKey(bytes32 _entropy) internal {
    bytes32 newKey = bytes32(Sapphire.randomBytes(32, abi.encodePacked(_entropy)));

    _ringKeys.push(newKey);
    uint newIndex = _ringKeys.length - 1; // Calculate newIndex after pushing

    _lastRingKeyUpdate = block.timestamp;

    emit ActualRingKeyRenewed(newIndex);
}

[batmanBinary]

@party-for-illuminati ?

[party-for-illuminati]

@party-for-illuminati ?

How is index calculation incorrect?

Example 1:

array = []
newIndex = array.length = 0
array.push(test)
array[newIndex] is test

Example 2:

array = [test0]
newIndex = array.length = 1
array.push(test1)
array[newIndex] is test1