Squeezing drips from a sender can be front-run and prevented by the sender

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L411 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L461

Vulnerability details

Squeezing drips from a sender requires providing the sequence of drips configurations (see NatSpec description in L337-L338):

/// It can start at an arbitrary past configuration, but must describe all the configurations /// which have been used since then including the current one, in the chronological order.

The provided history entries are hashed and verified against the sender's dripsHistoryHash.

However, the sender can prevent a receiver from squeezing drips by front-running the squeeze transaction and adding a new configuration. Adding a new configuration updates the current dripsHistoryHash and invalidates the historyHash provided by the receiver when squeezing. The receiver will then fail the drips history verification in L461 and the squeeze will fail.

Impact

A sender can prevent its drip receivers from squeezing by front-running the squeeze transaction and adding a new configuration.

Proof of Concept

Drips.sol#L411

392: function _squeezeDripsResult(
393:     uint256 userId,
394:     uint256 assetId,
395:     uint256 senderId,
396:     bytes32 historyHash,
397:     DripsHistory[] memory dripsHistory
398: )
399:     internal
400:     view
401:     returns (
402:         uint128 amt,
403:         uint256 squeezedNum,
404:         uint256[] memory squeezedRevIdxs,
405:         bytes32[] memory historyHashes,
406:         uint256 currCycleConfigs
407:     )
408: {
409:     {
410:         DripsState storage sender = _dripsStorage().states[assetId][senderId];
411:         historyHashes = _verifyDripsHistory(historyHash, dripsHistory, sender.dripsHistoryHash);
412:         // If the last update was not in the current cycle,
413:         // there's only the single latest history entry to squeeze in the current cycle.
414:         currCycleConfigs = 1;
415:         // slither-disable-next-line timestamp
416:         if (sender.updateTime >= _currCycleStart()) currCycleConfigs = sender.currCycleConfigs;
417:     }
...      // [...]

Drips.sol#L461

444: function _verifyDripsHistory(
445:     bytes32 historyHash,
446:     DripsHistory[] memory dripsHistory,
447:     bytes32 finalHistoryHash
448: ) private pure returns (bytes32[] memory historyHashes) {
449:     historyHashes = new bytes32[](dripsHistory.length);
450:     for (uint256 i = 0; i < dripsHistory.length; i++) {
451:         DripsHistory memory drips = dripsHistory[i];
452:         bytes32 dripsHash = drips.dripsHash;
453:         if (drips.receivers.length != 0) {
454:             require(dripsHash == 0, "Drips history entry with hash and receivers");
455:             dripsHash = _hashDrips(drips.receivers);
456:         }
457:         historyHashes[i] = historyHash;
458:         historyHash = _hashDripsHistory(historyHash, dripsHash, drips.updateTime, drips.maxEnd);
459:     }
460:     // slither-disable-next-line incorrect-equality,timestamp
461:     require(historyHash == finalHistoryHash, "Invalid drips history");
462: }

Tools Used

Manual review

Recommended mitigation steps

Consider allowing a receiver to squeeze drips from a sender up until the current timestamp.

[GalloDaSballo]

See #218 and #107, separate for now

[xmxanuel]

Technically the described attack would work.

It is also related to the trust assumption between the sender an receiver. Which are anyway given in a certain form.

I am unsure about the proposed solutions. Currently, we only store the latest dripsHistory hash on-chain. For allowing to squeeze until a specific timestamp, it might be necessary to have the full list of historic hashes on-chain.

[CodeSandwich]

[disagree with severity: QA] This attack is in fact possible, but it only allows postponing collecting funds until the end of the cycle. The proposed solution would probably require a lot of storage, so it's probably not worth introducing.

[c4-sponsor]

CodeSandwich marked the issue as disagree with severity

[GalloDaSballo]

I believe this boils down to the same underlying issue highlighted in #107

[GalloDaSballo]

This is basically the other side of #107, I'll think about it, but due to logical equivalency and the underlying issue being the same, I think I'll duplicate this one as well

[c4-judge]

GalloDaSballo marked the issue as duplicate of #201

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[GalloDaSballo]

During Post-Judging QA it was brought to my attention that this is a separate finding, have looked at this one again

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[GalloDaSballo]

In order to judge this issue I spoke with 4 other judges as well as the Sponsor.

This is a difficult decision because of the unclear expectations as to whether the sender is bening towards the receiver or not.

Because of the uncertainty, and the possibility of performing the grief, the finding is valid.

The issue is in terms of determining it's severity.

The attacker doesn't gain anything

One of the most interesting arguments is the idea that "the attacker doesn't gain anything", which can be falsified in scenarios in which squeezing would have helped reach a threshold of tokens in circulation, out of vesting or for similar purposes.

If we try hard enough, through multiple people's effort, we can come up with a scenario in which squeezing could make a difference between having enough votes for a Governance Operation, or could offer enough collateral to avoid a liquidation or some level of risk.

Those scenarios, while unlikely, can help discredit the idea that "the attacker doesn't gain anything".

The grief is limited

On the other hand, we must acknowledge that the attack / griefing, is limited:

• The sender is the only privileged account that can perform the grief
• Squeezing is denied until the next cycle, which based on configuration, will be known, and will be between 1 week and 1 month.

These can be viewed as additional external requirements, which reduce the impact / likelihood of the finding

The Squeeze is Squoze

In spite of the external requirements, the finding is showing how the functionality of Squeezing can be denied by the sender.

I believe that if anybody could grief squeezing, we would not hesitate in awarding Medium Severity and perhaps we'd be discussing around Med / High.

However, in this case, the only account that can perform the grief is the sender.

At the same time, the goal of the system is to allow Squeezing, which per the discussion above, given the finding can be prevented until a drip has moved to a newer crycle.

Last Minute Coding

I went into the tests and wrote the following illustratory cases

function testSenderCanStopAfter() public {
        uint128 amt = cycleSecs;
        setDrips(sender, 0, amt, recv(receiver, 1), cycleSecs);
        DripsHistory[] memory history = hist(sender);
        skip(1);
        squeezeDrips(receiver, sender, history, 1);
        setDrips(sender, amt - 1, 0, recv(receiver, 1), 0);

        // Squeeze again
        DripsHistory[] memory history2 = hist(history, sender);
        squeezeDrips(receiver, sender, history2, 0);

        skipToCycleEnd();
        receiveDrips(receiver, 0);
    }

    function testSenderCannotChangeTheirMind() public {
        uint128 amt = cycleSecs;
        setDrips(sender, 0, amt, recv(receiver, 1), cycleSecs);
        DripsHistory[] memory history = hist(sender);
        skip(1);
        setDrips(sender, amt - 1, 0, recv(receiver, 1), 0);

        // Squeeze First time
        DripsHistory[] memory history2 = hist(history, sender);
        squeezeDrips(receiver, sender, history2, 1);

        skipToCycleEnd();
        receiveDrips(receiver, 0);
    }

Ultimately we can see that what is owed to the recipient can never be taken back, however the sender can, through the grief shown in the above finding, prevent the tokens from being received until the end.

Conclusion

Given the information above, being mindful of:

• A potentially severe risk, with very low likelyhood
• The temporary breaking of the functionality of squeezing, which was meant to allow by-the-second claims

I agree with Medium Severity

[c4-judge]

GalloDaSballo marked the issue as selected for report

[c4-judge]

GalloDaSballo marked the issue as primary issue