code-423n4 / 2023-06-xeth-mitigation-findings

0 stars 0 forks source link

M-05 Unmitigated #27

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

Vulnerability details

Mitigation of M-05: Issue NOT mitigated

Mitigated issue

M-05: Virgin stake can claim all drops Fix: https://github.com/code-423n4/2023-05-xeth/commit/aebc3244cbb0deb67f3cdef160b390da27888de7

The issue is that if dripping is enabled when totalSupply() == 0 the entire amount dripped will immediately accrue to the first stake.

Mitigation review - dripping is still implicit when totalSupply() == 0

The drip accrual is now skipped, i.e. _accrueDrip() simply returns, when totalSupply() == 0. However, the drip is implicitly accrued at a constant rate per time, by actually adding the dripped amount only at discrete points in time, the last point in time of which is lastReport, which is what happens in _accrueDrip(). But this means that simply skipping this drip accrual when totalSupply() == 0 only defers explicit drip accrual to the next time, but will still drip the same amount because it is calculated from the same lastReport. That is, the drip is not truly suspended until the first stake. The attack will still work because the drip will just accrue on the unstake instead, even when it is unstaked immediately after the first stake.

Example

  1. Start drip at t = 0. lastReport = 0 and totalSupply() == 0.
  2. First stake at t = T. totalSupply() == 0 so _accrueDrip() immediately returns, which means that nothing is dripped and that lastReport remains 0.
  3. Unstake at t = T. totalSupply() > 0 so 100 * dripRatePerBlock is dripped, which goes to the unstaker.

The issue is in step 2, that lastReport remains 0. In order to truly not drip, lastReport must be reset without adding any dripped amount. Then the drip is neither implicitly nor explicitly happening unless something has been staked, i.e. totalSupply() > 0.

Suggested mitigation

This can be achieved by, in _accrueDrip():

if (!dripEnabled) return;
if (totalSupply() == 0) {
    lastReport = block.number;
    return;
}

Now, startDrip() enables drip but the drip only truly starts after the first stake. It also automatically stops when everything is unstaked, i.e. the drip only kicks in when something is staked.

kirk-baird commented 1 year ago

Nullified in favour of #22

c4-judge commented 1 year ago

kirk-baird marked the issue as nullified