`_receiveDripsResult()` overcounts `amtPerCycle`

[code423n4]

Lines of code

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

Vulnerability details

Impact

Drips results will be accounted for wrongly and hence users will receive more drips than they should.

Proof of Concept

According to the whitepaper, amtDeltas stored at each cycle is the value relative to the previous cycle. The stored delta for a cycle in the timeline can be seen as the sum of individuals sender deltas for that cycle.

For example, if our cycle delta value contains

1 5 3 0 5

then the delta received amount at each point would be,

1 6 9 9 14

and the total amount we should receive at each point would be

1 7 16 25 39

However, in our _receiveDripsResult() implementation, not only does it add state.amtDeltas[cycle].thisCycle, it also adds state.amtDeltas[cycle].nextCycle to amtPerCycle. This means that the value of every cycle except for the first and last is accounted for twice. This would count the amount per cycle that we should receive.

function _receiveDripsResult(uint256 userId, uint256 assetId, uint32 maxCycles)
    internal
    view
    returns (
        uint128 receivedAmt,
        uint32 receivableCycles,
        uint32 fromCycle,
        uint32 toCycle,
        int128 amtPerCycle
    )
{
    (fromCycle, toCycle) = _receivableDripsCyclesRange(userId, assetId);
    if (toCycle - fromCycle > maxCycles) {
        receivableCycles = toCycle - fromCycle - maxCycles;
        toCycle -= receivableCycles;
    }
    DripsState storage state = _dripsStorage().states[assetId][userId];
    for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
        amtPerCycle += state.amtDeltas[cycle].thisCycle;
        receivedAmt += uint128(amtPerCycle);
        amtPerCycle += state.amtDeltas[cycle].nextCycle;
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

We should only add state.amtDeltas[cycle].thisCycle for each cycle.

for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
    amtPerCycle += state.amtDeltas[cycle].thisCycle;
    receivedAmt += uint128(amtPerCycle);
-   amtPerCycle += state.amtDeltas[cycle].nextCycle;
}  

[xmxanuel]

This is not correct. For getting the correct amtPerCycle thisCycle and nextCycle needs to be considered.

In the last iteration of the for loop. The nextCycle is not added to the receivedAmt. (This only happens in the next iteration of the loop). So a double accounting doesn't occur.

[CodeSandwich]

[dispute validity] What Manuel wrote. The proposed fix breaks 14 tests.

[c4-sponsor]

xmxanuel marked the issue as sponsor disputed

[GalloDaSballo]

Closing for lack of proof, I believe if we went out of bounds we would add 0 meaning it should be fine, recommend the Warden to follow up with a POC if they think this is valid

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof