ctf_sec - AutoRoller.sol#roll can revert if lastSettle is zero because solmate ERC4626 deposit revert if previewDeposit returns 0

[sherlock-admin]

ctf_sec

medium

AutoRoller.sol#roll can revert if lastSettle is zero because solmate ERC4626 deposit revert if previewDeposit returns 0

Summary

AutoRoller.sol#roll can revert if lastSettle is zero because solmate ERC4626 deposit revert if previewDeposit returns 0

Vulnerability Detail

let us look into the implementation of function roll()

  /// @notice Roll into the next Series if there isn't an active series and the cooldown period has elapsed.
  function roll() external {
      if (maturity != MATURITY_NOT_SET) revert RollWindowNotOpen();

      if (lastSettle == 0) {
          // If this is the first roll, lock some shares in by minting them for the zero address.
          // This prevents the contract from reaching an empty state during future active periods.
          deposit(firstDeposit, address(0));
      } else if (lastSettle + cooldown > block.timestamp) {
          revert RollWindowNotOpen();
      }

      lastRoller = msg.sender;
      adapter.openSponsorWindow();
  }

note, if lastSettle is 0, we deposit a small amount of token and mint shares to address(0)

deposit(firstDeposit, address(0));

First deposit is a fairly small amount:

firstDeposit  = (0.01e18 - 1) / scalingFactor + 1;

We can deposit from ERC4626 implementation:

function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    // Need to transfer before minting or ERC777s could reenter.
    asset.safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
}

note the restriction:

// Check for rounding error since we round down in previewDeposit.
require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

// Need to transfer before minting or ERC777s could reenter.
asset.safeTransferFrom(msg.sender, address(this), assets);

if previewDeposit returns 0 shares, transaction revert. Can previewDeposit returns 0 shares? it is very possible.

function previewDeposit(uint256 assets) public view override returns (uint256) {
    if (maturity == MATURITY_NOT_SET) {
        return super.previewDeposit(assets);
    } else {
        Space _space = space;
        (uint256 ptReserves, uint256 targetReserves) = _getSpaceReserves();

        // Calculate how much Target we'll end up joining the pool with, and use that to preview minted LP shares.
        uint256 previewedLPBal = (assets - _getTargetForIssuance(ptReserves, targetReserves, assets, adapter.scaleStored()))
            .mulDivDown(_space.adjustedTotalSupply(), targetReserves);

        // Shares represent proportional ownership of LP shares the vault holds.
        return previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this)));
    }
}

If (previewedLPBal * total) / space balance is truncated to 0, transaction revert. _space.balanceOf can certainly be inflated if malicious actor send the space token to the address manually. Or previewedLPBal * total could just be small and the division is truncated to 0.

Impact

calling roll would revert and the new sponsored series cannot be started properly.

Code Snippet

https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L152-L168

https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L416-L435

Tool used

Manual Review

Recommendation

We recommend the project not deposit a such small amount, or there could be a function that let admin gradually control how many tokens should we put in the first deposit.

[jparklev]

While this is an interesting observation and it got us thinking, we don't think that it's something to be concerned about for two reasons:

1. roll would be unaffected since the deposit before roll happens during a cooldown period, where the previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this))) line is never executed
2. previewedLPBal is a function of total supply, so if _space.balanceOf(address(this)) were made larger, so to would previewedLPBal

We aren't yet on 100% confidence here tho, so if there were to be a test case demonstrating a concrete example of how this could happen, it would be much appreciated 🙏

[aktech297]

I believe if this needs to happen, user have to waste a large amount of fund to do it for no personal gain. I see the #41 has fix for this.
Further adding, a simple test is needed to ensure the function flow to confirm when the else part would be executed. I don't see the actual flow from the explanation.

 else {
    Space _space = space;
    (uint256 ptReserves, uint256 targetReserves) = _getSpaceReserves();

    // Calculate how much Target we'll end up joining the pool with, and use that to preview minted LP shares.
    uint256 previewedLPBal = (assets - _getTargetForIssuance(ptReserves, targetReserves, assets, adapter.scaleStored()))
        .mulDivDown(_space.adjustedTotalSupply(), targetReserves);

    // Shares represent proportional ownership of LP shares the vault holds.
    return previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this)));
}

[jacksanford1]

Bringing in a comment from the protocol team:

We agreed with ak1's comment on this one and, while in theory valid, considered it a prohibitively expensive attack.

Categorizing it as acknowledged.