Adding balance to accumulator does not depend on the current drawId, while documentation says it does

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L64-L112

Vulnerability details

Impact

In documentation protocol states that : To compute the allocated contribution for a draw d we'd compute the integral of curve c(d)=−t∗ln(α)∗α^d from lastdraw d(old) to d(new), and which is equal to −t∗ α^d(old) + t∗ α^d(new). Which clearly shows that contribution only does not depend the interval of lastdraw to new draw while it also depends the current draw.

Now Coming to actual implementation , the implemented code does not depend on the current drawId, it only depends on the interval between new draw and completed draw.

if vaults are not producing high yields and new contributions are not good enough This calculation can break protocol and will consume reserve continuously as it will calculate balance based on the interval between draws[most probably high area region of contribution curve].

Proof of Concept

we have 2 identical accumulator , accumulator1 & accumulator2. Both have no balance in starting. Now suppose after different scenarios of contribution both accumulators have same available balance i.e after x1 draw in accumulator1 and x2 draw in accumulator2 both have same available balance and now for next draw both accumulators have same interval gap [newdraw-lastcompletedDraw]

File: src/libraries/DrawAccumulatorLib.sol

function add(
    Accumulator storage accumulator,
    uint256 _amount,
    uint16 _drawId,
    SD59x18 _alpha
  ) internal returns (bool) {
    if (_drawId == 0) {
      revert AddToDrawZero();
    }
    RingBufferInfo memory ringBufferInfo = accumulator.ringBufferInfo;

    uint256 newestIndex = RingBufferLib.newestIndex(ringBufferInfo.nextIndex, MAX_CARDINALITY);
    uint16 newestDrawId_ = accumulator.drawRingBuffer[newestIndex];

    if (_drawId < newestDrawId_) {
      revert DrawClosed(_drawId, newestDrawId_);
    }

    Observation memory newestObservation_ = accumulator.observations[newestDrawId_];
    if (_drawId != newestDrawId_) {
      uint256 relativeDraw = _drawId - newestDrawId_;

      uint256 remainingAmount = integrateInf(_alpha, relativeDraw, newestObservation_.available);
      uint256 disbursedAmount = integrate(_alpha, 0, relativeDraw, newestObservation_.available);
      uint256 remainder = newestObservation_.available - (remainingAmount + disbursedAmount);

      accumulator.drawRingBuffer[ringBufferInfo.nextIndex] = _drawId;
      accumulator.observations[_drawId] = Observation({
        available: uint96(_amount + remainingAmount),
        disbursed: uint168(newestObservation_.disbursed + disbursedAmount + remainder)
      });
      uint16 nextIndex = uint16(RingBufferLib.nextIndex(ringBufferInfo.nextIndex, MAX_CARDINALITY));
      uint16 cardinality = ringBufferInfo.cardinality;
      if (ringBufferInfo.cardinality < MAX_CARDINALITY) {
        cardinality += 1;
      }
      accumulator.ringBufferInfo = RingBufferInfo({
        nextIndex: nextIndex,
        cardinality: cardinality
      });
      return true;
    } else {
      accumulator.observations[newestDrawId_] = Observation({
        available: uint96(newestObservation_.available + _amount),
        disbursed: newestObservation_.disbursed
      });
      return false;
    }
  }

function testAttack() public {

  DrawAccumulatorLib.add(accumulator1, 1000, 5,alpha);
  DrawAccumulatorLib.add(accumulator2, 1000, 10,alpha);

  assertEq(accumulator1.observations[5].available, accumulator2.observations[10].available);

  DrawAccumulatorLib.add(accumulator1,1500, 9,alpha);
  DrawAccumulatorLib.add(accumulator2,1500, 14,alpha);
  assertEq(accumulator1.observations[9].available, accumulator2.observations[14].available);

}

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L64C1-L112C4

This is simple demonstration that shows claimed issue. In General contribution is not dependent on draw it only depends on the interval between draw.

Tools Used

Foundry Testing

Recommended Mitigation Steps

With the current implementation do the origin shifting of contribution curve properly to lastDraw or Find other way to implement it

Assessed type

Math

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[asselstine]

The given test passes. Perhaps the warden is mistaken?

[c4-sponsor]

asselstine requested judge review

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[Picodes]

I don't see the issue: it seems normal as the distribution is exponential: the formula is the same if you replace with t with t^d(old), so the remaining amount of another draw so as long as amounts are updated it seems correct