code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L1103-L1114

Vulnerability details

Impact

In the _drippedAmt() function of the Drips.sol contract, the assembly is used to calculate the amount dripped over a time range. Inside the assembly amtPerCycle is calculated by multiplying cycleSecs and amtPerSec and dividing by _AMT_PER_SEC_MULTIPLIER to remove the decimal places introduced by amtPerSec. Due to the division amtPerCycle will be rounded down to the nearest uint value. And the rounded amtPerCycle is then multiplied by endedCycles to calculate amt value. Hence this multiplication enhances the rounding error introduced by the rounding down of the amtPerCycle. Thus calculated amt value will be less than precise value.

Similar rounding down is observed in the calculation of the amtEnd and amtStart.

Proof of Concept

Since the division by _AMT_PER_SEC_MULTIPLIER rounds down the amtPerCycle, its multiplication by endedCycles results in rounding error, which aggravates with the increasing endedCycles number. Hence the amt value is susceptible to rounding error.

    assembly {
        let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))
        // slither-disable-next-line divide-before-multiply
        let amtPerCycle := div(mul(cycleSecs, amtPerSec), _AMT_PER_SEC_MULTIPLIER)
        amt := mul(endedCycles, amtPerCycle)
        // slither-disable-next-line weak-prng
        let amtEnd := div(mul(mod(end, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
        amt := add(amt, amtEnd)
        // slither-disable-next-line weak-prng
        let amtStart := div(mul(mod(start, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
        amt := sub(amt, amtStart)
    }

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L1103-L1114

As shown above the calculation of amtEnd and amtStart also introduces the rounding error to the final calculation of the amt value.

Tools Used

Manual and VS Code

Recommended Mitigation Steps

It is not required to eliminate the decimal places introduced by amtPerSec (by dividing by _AMT_PER_SEC_MULTIPLIER) during each step of the amt value calculation. The amt value can be calculated with the decimal places at each step of the calculation and ones the final amt value is determined then can divide by _AMT_PER_SEC_MULTIPLIER to remove the decimal values and return the precise amt value.

Code can be rewritten as follows:

    assembly {
        let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))
        // slither-disable-next-line divide-before-multiply
        let amtPerCycle := mul(cycleSecs, amtPerSec) //@audit-issue - this triggers a rounding error.
        amt := mul(endedCycles, amtPerCycle) //@audit-issue - since rounded value is multiplied here if the endedCycles number is high the rounding error will be large.
        // slither-disable-next-line weak-prng
        let amtEnd := mul(mod(end, cycleSecs), amtPerSec) //@audit-issue - rounding error
        amt := add(amt, amtEnd)
        // slither-disable-next-line weak-prng
        let amtStart := mul(mod(start, cycleSecs), amtPerSec) //@audit-issue - rounding error.
        amt := sub(amt, amtStart)
        amt := div(amt, _AMT_PER_SEC_MULTIPLIER)
    }

GalloDaSballo commented 1 year ago

While the order is changed, I believe the last subtraction makes it so that the any rounding error is not lost, but simply will be paid at the end: https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L1113

The submission also seems to miss any quantifiable risk so I would downgrade to QA at most.

Will flag but most likely invalid

CodeSandwich commented 1 year ago

[dispute validity] This is intended, the dripped amount is rounded down at the end of the cycle, so each cycle drips the same amount.

c4-sponsor commented 1 year ago

CodeSandwich marked the issue as sponsor disputed

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

code-423n4 / 2023-01-drips-findings