074.md

Quiet Seafoam Carp

Medium

NotInAuction modifier in Pool contract always passes requirement

Summary

Pool.sol:NotInAuction() modifier always passes requirement because of wrong assumption about BondToken.currentPeriod and current state of auction in modifier check (current period auction always equals address(0)).

Root Cause

In Pool.sol:NotInAuction() modifier wrong check if auction is running:

/**
  * @dev Modifier to prevent a function from being called during an ongoing auction.
  */
modifier NotInAuction() {
  (uint256 currentPeriod,) = bondToken.globalPool();
  //auctions[currentPeriod] == address(0) always
  require(auctions[currentPeriod] == address(0), AuctionIsOngoing());
  _;
}

Basically auctions[currentPeriod] always equals address(0) because BondToken.currentPeriod being updated during Pool.startAuction() through bondToken.increaseIndexedAssetPeriod():

/**
  * @dev Starts an auction for the current period.
  */
function startAuction() external whenNotPaused() {
  // Check if distribution period has passed
  require(lastDistribution + distributionPeriod < block.timestamp, DistributionPeriodNotPassed());

  // Check if auction period hasn't passed
  require(lastDistribution + distributionPeriod + auctionPeriod >= block.timestamp, AuctionPeriodPassed());

  // Check if auction for current period has already started
  (uint256 currentPeriod,) = bondToken.globalPool();
  require(auctions[currentPeriod] == address(0), AuctionAlreadyStarted());

  uint8 bondDecimals = bondToken.decimals();
  uint8 sharesDecimals = bondToken.SHARES_DECIMALS();
  uint8 maxDecimals = bondDecimals > sharesDecimals ? bondDecimals : sharesDecimals;

  uint256 normalizedTotalSupply = bondToken.totalSupply().normalizeAmount(bondDecimals, maxDecimals);
  uint256 normalizedShares = sharesPerToken.normalizeAmount(sharesDecimals, maxDecimals);

  // Calculate the coupon amount to distribute
  uint256 couponAmountToDistribute = (normalizedTotalSupply * normalizedShares)
      .toBaseUnit(maxDecimals * 2 - IERC20(couponToken).safeDecimals());

  auctions[currentPeriod] = Utils.deploy(
    address(new Auction()),
    abi.encodeWithSelector(
      Auction.initialize.selector,
      address(couponToken),
      address(reserveToken),
      couponAmountToDistribute,
      block.timestamp + auctionPeriod,
      1000,
      address(this),
      poolSaleLimit
    )
  );

  //Increase the bond token period
  //@audit auctions[currentPeriod] again equals address(0)
  bondToken.increaseIndexedAssetPeriod(sharesPerToken);

  // Update last distribution time
  lastDistribution = block.timestamp;
}

Internal Pre-conditions

No response

External Pre-conditions

No response

Attack Path

No response

Impact

NotInAuction modifier always passes requirement though should block functions like setDistributionPeriod(), setAuctionPeriod() and setSharesPerToken() during auction.

PoC

Add this test in the /test/Pool.t.sol:

function testNotInAuctionModifier() public {
  vm.startPrank(governance);
  //create pool
  uint256 totalUnderlyingAssets = uint256(1000000000);
  Token rToken = Token(params.reserveToken);
  rToken.mint(governance, totalUnderlyingAssets);
  rToken.approve(address(poolFactory), totalUnderlyingAssets);
  Pool _pool = Pool(poolFactory.createPool(
    params, 
    totalUnderlyingAssets,   //TotalUnderlyingAssets
    25000000000,             //DebtAssets
    1000000000,              //LeverageAssets
    "", "salt", "", "", false));
  //updating sharesPerToken succesfully auction not started yet
  //NotInAuction modifier not reverting as defined
  _pool.setSharesPerToken(params.sharesPerToken);
  //updating auction period to pass AuctionPeriodPassed()
  //NotInAuction modifier not reverting as defined
  _pool.setAuctionPeriod(1000);
  //add a bit of time to pass distribution time
  vm.warp(10);
  //start auction
  //bond token period increased
  _pool.startAuction();
  //updating sharesPerToken succesfully even though auction started
  //NotInAuction modifier not reverting THOUGH HE SHOULD
  _pool.setSharesPerToken(params.sharesPerToken);
  //add time to end auction
  vm.warp(2000);
  //get current auction and end it
  Auction _auction = Auction(_pool.auctions(0));
  _auction.endAuction();
  //updating sharesPerToken again succesfully even though auction ended
  //NotInAuction modifier not reverting as defined though
  _pool.setSharesPerToken(params.sharesPerToken);
  vm.stopPrank();
}

In cmd run this command:

forge test -vv --mt testNotInAuctionModifier

Output:

Ran 1 test for test/Pool.t.sol:PoolTest
[PASS] testNotInAuctionModifier() (gas: 3859123)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.95ms (1.43ms CPU time)

Mitigation

Update NotInAuction modifier like this:

/**
  * @dev Modifier to prevent a function from being called during an ongoing auction.
  */
modifier NotInAuction() {
  (uint256 currentPeriod,) = bondToken.globalPool();
  if (currentPeriod != 0) {
    Auction currentAuction = Auction(auctions[currentPeriod - 1]);
    Auction.State currentAuctionState = currentAuction.state();
    require(
      currentAuctionState == Auction.State.FAILED_UNDERSOLD
      ||
      currentAuctionState == Auction.State.FAILED_POOL_SALE_LIMIT
      ||
      currentAuctionState == Auction.State.SUCCEEDED,
      AuctionIsOngoing()
    );
  }
  _;
}