SlabMachine.sol

[HIGH] Loss of user funds

Potential for failed pulls without proper error handling. The fulfillRandomWords function doesn't properly validate that the requested rarity has available tokens before attempting to pull.

Vulnerable Code:

function fulfillRandomWords(uint _requestId, uint[] calldata _randomWords) internal override {
  // ... validation logic ...
    
  for (uint i; i < request.slabs; ++i) {
    SlabRarity rarity = _determineRarity(_randomWords[i * 2]);
        
    uint remainingRarityTokens = _rarityTokens[rarity].length();
        
    // If no cards are remaining for the rarity, then we need to exit
    if (remainingRarityTokens == 0) {
      emit SlabPullFailed(rarity, request.creator);
      continue;  // ← User loses money but gets nothing
    }
        
    // ... rest of logic ...
  }
}

Issue: Users pay for pulls but may receive nothing if their determined rarity is out of stock.

Recommended Fix:

• Refund users when their determined rarity is unavailable

function fulfillRandomWords(uint _requestId, uint[] calldata _randomWords) internal override {
  // ... validation logic ...
    
  for (uint i; i < request.slabs; ++i) {
    SlabRarity rarity = _determineRarity(_randomWords[i * 2]);
        
    uint remainingRarityTokens = _rarityTokens[rarity].length();
        
    // If no cards are remaining for the rarity, then we need to exit
    if (remainingRarityTokens == 0) {
      emit SlabPullFailed(rarity, request.creator);
      continue;
    }
        
    // ... rest of logic ...
  }
}

[HIGH] Unbounded Loop in shutdown Function

Potential self-inflicted DoS if contract holds too many NFTs. The shutdown function iterates through all NFTs without gas limit considerations. This could result in cards and USDC being trapped inside the machine as the iteration loop would result in an Out-Of-Gas exception and prevent execution.

Vulnerable Code:

function shutdown(address _tokenRecipient, address _usdcRecipient) public onlyOwner {
  // ... validation logic ...
  
  // Withdraw remaining NFTs. We run in reverse order as otherwise the index value
  // would be manipulated during withdrawals and revert.
  uint slabBalance = slabs.balanceOf(address(this));
  for (uint i = slabBalance; i > 0; --i) {
      uint tokenId = slabs.tokenOfOwnerByIndex(address(this), i - 1);
      slabs.transferFrom(address(this), _tokenRecipient, tokenId);
  }

  // Withdraw remaining USDC
  uint usdcBalance = usdc.balanceOf(address(this));
  if (usdcBalance > 0) {
      usdc.transfer(_usdcRecipient, usdcBalance);
  }
}

Recommended Fix:

• Separate the calls, having the shutdown call handle just the logic to stop the machine
• Implement batch processing with gas limits

function shutdown(address _tokenRecipient, address _usdcRecipient) public onlyOwner {
  // Ensure that the machine has not already been shutdown
  if (isShutdown) {
    revert SlabMachineIsShutdown();
  }

  // Set the machine to be both closed and shutdown
  isClosed = true;
  isShutdown = true;

  emit SlabMachineShutdown(slabBalance, usdcBalance);
}

function withdrawSlabs(address _tokenRecipient, uint[] calldata _tokenIds) public onlyOwner {
  require(isShutdown);
  
  for (uint i = 0; i > _tokenIds.length; ++i) {
    slabs.transferFrom(address(this), _tokenRecipient, _tokenIds[i]);
  }
}

function withdrawUSDC(address _usdcRecipient) public onlyOwner {
  require(isShutdown);

  // Withdraw remaining USDC
  usdc.transfer(_usdcRecipient, usdc.balanceOf(address(this)));
}

[LOW] Missing Access Control in setRarityCalculator

Owner can manipulate rarity distribution to their advantage. The setRarityCalculator function allows the owner to change the rarity calculation contract at any time, potentially manipulating the odds in their favor.

function setRarityCalculator(RarityCalculator _rarityCalculator) public onlyOwner {
    rarityCalculator = RarityCalculator(_rarityCalculator);
}

Exploit Scenario:

1. Owner deploys a malicious rarity calculator that always returns high-value rarities
2. Owner changes the rarity calculator to the malicious one
3. Owner pulls NFTs and gets only high-value rarities
4. Owner changes back to legitimate calculator

Recommended Fix:

• Add events for transparency
• Consider locking the rarity calculator until machine is shutdown or paused

function setRarityCalculator(RarityCalculator _rarityCalculator) public onlyOwner {
  // Ensure that the machine is either shutdown or closed before updating
  if (isShutdown || isClosed) {
    revert CannotUpdateRarityCalculator();
  }

  rarityCalculator = RarityCalculator(_rarityCalculator);
  emit RarityCalculatorUpdated(_rarityCalculator);
}

[INFO] Make MAX_PULLS updatable

We would recommend making the MAX_PULLS parameter upgradable by the owner of the contract. This would help promote protocol configuration without redeployment.

[INFO] Use SafeTransferLib for ERC20 and ERC721 transfers

For better safety in transfers, it is recommended to use SafeTransferLib where possible, and for low-level transfer calls the callback booleans should be validated against.