Reserve contest - JTJabba's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L202-L205 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L392-L393 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L406-L418

Vulnerability details

Impact

RToken::issue and RToken::vest are used to mint new RTokens from their underlying collateral. Issuance per block is capped to a percent of the total supply in issue by adding the issuance to an account's issue queue that can't be vested until the time provided by whenFinished if issuance can't be completed that block.

whenFinished calculates the end block by adding the time needed for the amount of RTokens to be issued at the allowed issuance rate to the time delay for all other vesting issuance.

File: protocol\contracts\p1\RToken.sol

342:     /// Add amtRToken's worth of issuance delay to allVestAt, and return the resulting finish time.
343:     /// @return finished D18{block} The new value of allVestAt
344:     function whenFinished(uint256 amtRToken) private returns (uint192 finished) {
345:         // Calculate the issuance rate (if this is the first issuance in the block)
346:         if (lastIssRateBlock < block.number) {
347:             lastIssRateBlock = block.number;
348: 
349:             // D18{rTok/block} = D18{1/block} * D18{rTok} / D18{1}
350:             // uint192 downcast is safe, max value representations are 1e18 * 1e48 / 1e18
351:             lastIssRate = uint192((issuanceRate * totalSupply()) / FIX_ONE);
352:             // uint192(<) is equivalent to Fix.lt
353:             if (lastIssRate < MIN_BLOCK_ISSUANCE_LIMIT) lastIssRate = MIN_BLOCK_ISSUANCE_LIMIT;
354:         }
355: 
356:         // make `before` be the fractional-block when this issuance should start;
357:         // before = max(allVestAt, block.number - 1)
358:         uint192 before = allVestAt; // D18{block number}
359:         // uint192 downcast is safe: block numbers are smaller than 1e38
360:         uint192 nowStart = uint192(FIX_ONE * (block.number - 1)); // D18{block} = D18{1} * {block}
361:         if (nowStart > before) before = nowStart;
362: 
363:         // finished: D18{block} = D18{block} + D18{1} * D18{RTok} / D18{rtok/block}
364:         // uint192() downcast here is safe because:
365:         //   lastIssRate is at least 1e24 (from MIN_ISS_RATE), and
366:         //   amtRToken is at most 1e48, so
367:         //   what's downcast is at most (1e18 * 1e48 / 1e24) = 1e38 < 2^192-1
368:         finished = before + uint192((FIX_ONE_256 * amtRToken + (lastIssRate - 1)) / lastIssRate);
369:         allVestAt = finished;
370:     }

Issuance can be cancelled, but allVestAt used for calculating future vesting times is not updated.

File: protocol\contracts\p1\RToken.sol

406:     function cancel(uint256 endId, bool earliest) external notFrozen {
407:         address account = _msgSender();
408:         IssueQueue storage queue = issueQueues[account];
409: 
410:         require(queue.left <= endId && endId <= queue.right, "out of range");
411: 
412:         // == Interactions ==
413:         if (earliest) {
414:             refundSpan(account, queue.left, endId);
415:         } else {
416:             refundSpan(account, endId, queue.right);
417:         }
418:     }

Sense allVestAt is not updated on cancellation, an attacker could freeze issuance for a long period of time by taking a flash loan of the collateral and repeatedly starting and cancelling a large issuance.

An account's queued issuance will also automatically be cancelled by vest and issue if basketHandler.nonce() changes. If allVestAt is already many blocks ahead, issuance won't be able to resume until the chain has caught up even though all previous issuance have effectively been cancelled.

Proof of Concept

Add the following test to RToken.test.ts. Please note this test could falsely fail or succeed in the future if the last argument of the IssuanceStarted event changes.

it('Vesting times should not be delayed by issuing and cancelling', async function () {
  const issueAmount: BigNumber = bn('100000e18')
  
  // Approve
  await token0.connect(addr1).approve(rToken.address, issueAmount)
  await token1.connect(addr1).approve(rToken.address, issueAmount)
  await token2.connect(addr1).approve(rToken.address, issueAmount)
  await token3.connect(addr1).approve(rToken.address, issueAmount)

  // Call issue and record emitted vestingEnd
  let firstVestingEnd = ""
  {
  const receipt = await (await rToken.connect(addr1)['issue(uint256)'](issueAmount)).wait()
  const IssuanceStartedArgs = receipt.events!.find(event => event.event == "IssuanceStarted")!.args!
  firstVestingEnd = IssuanceStartedArgs[IssuanceStartedArgs.length - 1].toString()
  }
  
  // Cancel
  await rToken.connect(addr1).cancel(1, true)

  // Approve
  await token0.connect(addr1).approve(rToken.address, issueAmount)
  await token1.connect(addr1).approve(rToken.address, issueAmount)
  await token2.connect(addr1).approve(rToken.address, issueAmount)
  await token3.connect(addr1).approve(rToken.address, issueAmount)

  // Call issue and record emitted vestingEnd
  let secondVestingEnd = ""
  {
    const receipt = await (await rToken.connect(addr1)['issue(uint256)'](issueAmount)).wait()
    const IssuanceStartedArgs = receipt.events!.find(event => event.event == "IssuanceStarted")!.args!
    secondVestingEnd = IssuanceStartedArgs[IssuanceStartedArgs.length - 1]
  }

  if (firstVestingEnd != secondVestingEnd){
    throw Error("First vestingEnd does not match second vestingEnd. Got '" + firstVestingEnd + "' and '" + secondVestingEnd + "'")
  }
})

Recommended Mitigation Steps

First, allVestAt should be reset once when basketHandler.nonce() changes so issuance can continue immediately.

cancel could be modified to update allVestAt as appropriate, but this would result in undesirable mechanics. Later issuance may vest before earlier issuance, and some users might be incentivized to restart issuance to vest sooner. This would also result in more volatile minting sense vesting times for issuance started after an issuance that is later cancelled wouldn't be moved up but future issuance would.

cancel could instead be modified to only be callable if basketHandler.nonce() has changed and and when vesting is complete, but funds would be locked for vesting. If this is done, issue should be updated to take a parameter for max allowed vesting time.