It is possible for the bits used to store `driverId` in `userId` to be corrupted

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/ImmutableSplitsDriver.sol#L53-L68

Vulnerability details

Title

It is possible for the bits used to store driverId in userId to be corrupted

Impact

While it is unlikely that counterSlot exceeds uint224, we must note that createSplits() is a function callable by by anyone, and it can be used to increase the value perpetually. Furthermore, should corruptibility happen, it will mess up access control granted to userIds.

Proof of Concept

userId stores driverId in the first 32 bits and _counterSlot in the next 224 bits. However, we note that _counterSlot is uint256 value.

function nextUserId() public view returns (uint256 userId) {
    return (uint256(driverId) << 224) + StorageSlot.getUint256Slot(_counterSlot).value;
}

Each time a split is created, value of counterSlot increases by one. This is a public function callable by any users. It is possible for the value to exceed uint224.max. If counterSlot > uint224.max, when we do the sum (uint256(driverId) << 224) + StorageSlot.getUint256Slot(_counterSlot).value, the driverId part userId will be corrupted and changes based on what is being added, but it is supposed to be fixed.

function createSplits(SplitsReceiver[] calldata receivers, UserMetadata[] calldata userMetadata)
    public
    whenNotPaused
    returns (uint256 userId)
{
    userId = nextUserId();
    StorageSlot.getUint256Slot(_counterSlot).value++;
    uint256 weightSum = 0;
    for (uint256 i = 0; i < receivers.length; i++) {
        weightSum += receivers[i].weight;
    }
    require(weightSum == totalSplitsWeight, "Invalid total receivers weight");
    emit CreatedSplits(userId, dripsHub.hashSplits(receivers));
    dripsHub.setSplits(userId, receivers);
    if (userMetadata.length > 0) dripsHub.emitUserMetadata(userId, userMetadata);
}

This is an issue as the modifiers onlyDriver and assertCallerIsDriver relies on the driverId to facilitates access control. It will mess up the permissions granted to userIds that are affected, possibly giving access to those not allowed, or removing access from those allowed.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using uint224 for _counterSlot instead.

[GalloDaSballo]

Thinking QA, because the addition will revert

[xmxanuel]

I think in this edge case counterSlot > (2^224-1) the addition will change the driverId as well and the setSplits will revert because the Driver doesn't own the driverID.

2^224 is very large and this scenario is very unlikely to reach.

We could add a cast check here just for cleanness in the code.

Recommendation: low

[CodeSandwich]

[dispute validity] The attack requires calling a contract 2^224 times. It's simply not possible, the entire humankind probably hasn't performed that many computational operations in its entire existence, let alone on a single blockchain. If you have a power capacity like this, you can just preimage the DripsHub address and drain all the assets held by it.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

Per the Sponsors comment this is a theoretical attack, I believe the finding can be awarded as informational, with an understanding that this cannot be performed in practice

[GalloDaSballo]

NC

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

2L 1NC after accounting for dups