OlaHamid - Key Collision in ethStreamEndingAtTick Mapping Allows Miscalculation of ETH Streaming Balances

[sherlock-admin2]

OlaHamid

High

Key Collision in ethStreamEndingAtTick Mapping Allows Miscalculation of ETH Streaming Balances

Summary

The ethStreamEndingAtTick mapping, defined here, uses streamLastTick as a key. This implementation leads to potential key collisions when multiple users create streams that resolve to the same streamLastTick value. Collisions result in incorrect ETH balances in the mapping, which impacts subsequent functionality such as forwardAll and increaseTicksAndFinishStreams.

---

Root Cause

The createStream function (source) calculates streamLastTick using the formula currentTick + streamLengthInTicks. If two users create streams with overlapping ticks or identical streamLengthInTicks while sharing the same currentTick, the resulting key (streamLastTick) will overwrite previous values in the mapping, conflating unrelated streams.

---

Internal Precondition

1. currentTick is a shared state variable, and multiple users rely on its value to calculate streamLastTick.
2. Users create streams with overlapping or identical streamLengthInTicks.

---

External Precondition

1. Multiple users are allowed to create streams.
2. The system assumes unique entries in the ethStreamEndingAtTick mapping.

---

Attack Path

1. User A creates a stream with streamLengthInTicks = 5.
   • The mapping updates ethStreamEndingAtTick[5].
2. User B creates a stream while the currentTick is still at the same state, and streamLengthInTicks = 3.
   • The mapping overwrites ethStreamEndingAtTick[5].

---

Impact

1. Incorrect ETH balances in ethStreamEndingAtTick affect calculations during stream processing (e.g., in forwardAll and increaseTicksAndFinishStreams).
2. May result in incorrect deductions, misallocations, or unexpected behavior for users and the DAO.

---

PoC

function test_createStream_keyCollision_withoutForwardAll() public {
    address user2 = makeAddr("User2");


    assertEq(nounsToken.ownerOf(1), streamCreator);

    vm.prank(streamCreator);
    escrow.createStream(1, 5); //

    // Mock an increment in currentTick (simulate time progression without forwardAll)
    uint256 mockedTick = escrow.currentTick() + 2;
    vm.store(address(escrow), keccak256("currentTick"), bytes32(uint256(mockedTick))); 

    // Step 2: User2 creates another stream with nounId = 2 and streamLength = 3
    assertEq(nounsToken.ownerOf(2), user2);

    vm.prank(user2);
    escrow.createStream(2, 3); // ethPerTick for this stream: msg.value / 3

    // Step 3: Verify that ethStreamEndingAtTick contains a collision
    uint128 expectedEthPerTick = uint128(msg.value / 5); // Original value for User1
    uint128 actualEthPerTick = escrow.ethStreamEndingAtTick(mockedTick + 3);

    assertEq(expectedEthPerTick, actualEthPerTick, "ethStreamEndingAtTick contains an incorrect value due to key collision");
}

---

Mitigation

Introduce a unique identifier for ethStreamEndingAtTick keys:

1. Use a composite key (e.g., keccak256(abi.encodePacked(streamLastTick, nounId))).
2. Refactor functions that rely on ethStreamEndingAtTick to use the updated structure.