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.