Badger Citadel contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L132-L136 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L830 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/interfaces/citadel/IVesting.sol#L5-L9

Vulnerability details

Impact

StakedCitadelVester contract doesn't support the IVesting interface, so the withdrawal of Citadel from the StakedCitadel contract will be impossible because the IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp); call will revert (because the function doesn't exist).

Tools Used

Remix & VS Code

Recommended Mitigation Steps

Change the vest function of the StakedCitadelVester contract to match the IVesting interface

Low and non-critical bugs

• There are multiple TODO comments that weren't removed - for example in the SupplySchedule, Funding, KnightingRound and GlobalAccessControl contracts

• The address _citadelPriceInAssetOracle parameter is missing in the comment before the initialize function of the Funding contract

• There is a mistake in the comment of the claim function of the StakedCitadelVester contract * @param amount The amount to transfer. If greater than the claimable amount, the maximum is transferred. - the minimum amount is transferred, not the maximum amount.

• The initializer function of multiple contracts is front-runnable, which means that an attacker can front run the transaction that you call it in and supply malicious parameters. This can be fixed using access controls - only the admin can call the initializer function (the admin is the msg.sender in the constructor)

• Use MAX_BPS instead of 10000 (magic number) in the setFundingPoolWeight function of the CitadelMinter contract (require(_weight <= 10000, "exceed max funding pool weight");)

• Missed checking that _minDiscount < _maxDiscount in the setDiscountLimits function:

  function setDiscountLimits(uint256 _minDiscount, uint256 _maxDiscount)
      external
      gacPausable
      onlyRole(CONTRACT_GOVERNANCE_ROLE)
  {
      require(_maxDiscount < MAX_BPS , "maxDiscount >= MAX_BPS");
      funding.minDiscount = _minDiscount;
      funding.maxDiscount = _maxDiscount;
      emit DiscountLimitsSet(_minDiscount, _maxDiscount);
  }

  and also in the setCitadelAssetPriceBounds function:

  function setCitadelAssetPriceBounds(uint256 _minPrice, uint256 _maxPrice)
      external
      gacPausable
      onlyRole(CONTRACT_GOVERNANCE_ROLE)
  {
      minCitadelPriceInAsset = _minPrice;
      maxCitadelPriceInAsset = _maxPrice;
      emit CitadelPriceBoundsSet(_minPrice, _maxPrice);
  }

  That can DoS the system (the citadel price in asset and discounts checks won't pass).

• Missing check that the new discount manager's address is not zero

  function setDiscountManager(address _discountManager)
      external
      gacPausable
      onlyRole(CONTRACT_GOVERNANCE_ROLE)
  {
      funding.discountManager = _discountManager;
      emit DiscountManagerSet(_discountManager);
  }

Gas optimizations

• Save a duration value in the VestingParams struct to avoid calculating it every time in the claimableBalance function (the calculation is vesting[recipient].unlockEnd - vesting[recipient].unlockBegin and it is a constant value)

• Delete the vesting params in the claim function if the claimed amount is equal to the locked amount (which means that the user claimed all of the locked amount and the vesting finished). That will trigger a gas refund.

• Save a saleEnd variable in the KnightingRound contract instead of calculating saleStart + saleDuration every time

• Setting value to zero is redundant (in the initialize function of the Funding contract) - minCitadelPriceInAsset = 0;. Variables in solidity are automatically initialized to their default value (which is zero in uint256's case).

• The funding.assetCumulativeFunded + _assetAmountIn calculation is done twice (both in the require and after it). You can save the result of it instead of calculating it twice.

  old code:

  require(
      funding.assetCumulativeFunded + _assetAmountIn <= funding.assetCap,
      "asset funding cap exceeded"
  );
  funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;

  new code:

  uint newAssetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;
  require(
      newAssetCumulativeFunded <= funding.assetCap,
      "asset funding cap exceeded"
  );
  funding.assetCumulativeFunded = newAssetCumulativeFunded;

• The hash value of constant expression can be calculated pre-deployment to save gas when calculating it (this can be done in the initialize function of StakedCitadel for example - calculate the value of keccak256("") before the deployment).

• Optimize the _depositFor function (use 2 variables instead of 3 variables) before changes:

  uint256 _pool = balance();
  uint256 _before = token.balanceOf(address(this));
  token.safeTransferFrom(msg.sender, address(this), _amount);
  uint256 _after = token.balanceOf(address(this));
  _mintSharesFor(_recipient, _after - _before, _pool);

  after changes:

  uint256 _pool = balance();
  token.safeTransferFrom(msg.sender, address(this), _amount);
  uint256 _delta = token.balanceOf(address(this)) - _pool;
  _mintSharesFor(_recipient, _delta_, _pool);

• In the Funding contract - use !citadelPriceFlag instead of citadelPriceFlag == false (this might look stupid but actually saves gas)

  before changes:

  modifier onlyWhenPriceNotFlagged() {
      require(
          citadelPriceFlag == false,
          "Funding: citadel price from oracle flagged and pending review"
      );
      _;
  }

  after changes:

  modifier onlyWhenPriceNotFlagged() {
      require(
          !citadelPriceFlag,
          "Funding: citadel price from oracle flagged and pending review"
      );
      _;
  }

• Use unchecked on calculations that you know for sure that won't overflow or underflow. For example in the getRemainingFundable function, we know that assetCumulativeFunded < assetCap so the calculation assetCumulativeFunded - assetCap won't underflow and unchecked can be used.

  function getRemainingFundable() external view returns (uint256 limitLeft_) {
      uint256 assetCumulativeFunded = funding.assetCumulativeFunded;
      uint256 assetCap = funding.assetCap;
      if (assetCumulativeFunded < assetCap) {
          unchecked {
              limitLeft_ = assetCap - assetCumulativeFunded;
          }
      }
  }

• The for loop in the getFundingPoolWeights and _transferToFundingPools functions of the CitadelMinter contract can be optimized in several ways:

  1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So for example in the loop above, the code can be optimized using uint i; instead of uint i = 0;.
  2. Use ++i instead of i++ to save some gas spent in every iteration.
  3. Use unchecked on the loop variable increment.

  code before:

  function getFundingPoolWeights()
      external
      view
      returns (address[] memory pools, uint256[] memory weights)
  {
      uint256 numPools = fundingPools.length();
      pools = new address[](numPools);
      weights = new uint256[](numPools);
      for (uint256 i = 0; i < numPools; i++) {
          address pool = fundingPools.at(i);
          uint256 weight = fundingPoolWeights[pool];
          pools[i] = pool;
          weights[i] = weight;
      }
  }

  code after:

  function getFundingPoolWeights()
      external
      view
      returns (address[] memory pools, uint256[] memory weights)
  {
      uint256 numPools = fundingPools.length();
      pools = new address[](numPools);
      weights = new uint256[](numPools);
      for (uint256 i; i < numPools; ) {
          address pool = fundingPools.at(i);
          uint256 weight = fundingPoolWeights[pool];
          pools[i] = pool;
          weights[i] = weight;
          unchecked { ++i; }
      }
  }

• Use delete instead of resetting the value of the weight in the _removeFundingPool function of CitadelMinter contract - to trigger gas refunds and save gas.

• Calculate epochStartTime and epochEndTime more efficiently in the getMintable function of the SupplySchedule contract

  old code:

  uint256 epochStartTime = cachedGlobalStartTimestamp;
  uint256 epochEndTime = epochStartTime + epochLength;
  for (uint256 i = startingEpoch; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
      
      uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;
      uint256 epochEndTime = cachedGlobalStartTimestamp + (i + 1) * epochLength;
      
      uint256 time = MathUpgradeable.min(block.timestamp, epochEndTime) -
          MathUpgradeable.max(lastMintTimestamp, epochStartTime);
      
      mintable += rate * time;
      
      unchecked { ++i; }
  }

  new code:

  for (uint256 i = startingEpoch; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
  
      
      uint256 time = MathUpgradeable.min(block.timestamp, epochEndTime) -
          MathUpgradeable.max(lastMintTimestamp, epochStartTime);
      
      mintable += rate * time;
      
      unchecked { 
          ++i;
          epochStartTime = epochEndTime;
          epochEndTime += epochLength;
      }
  }

• Check if the end time passed the current timestamp to avoid next iterations in the getMintable function of the SupplySchedule contract old code:

  for (uint256 i = startingEpoch; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
      
      uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;
      uint256 epochEndTime = cachedGlobalStartTimestamp + (i + 1) * epochLength;
      
      uint256 time = MathUpgradeable.min(block.timestamp, epochEndTime) -
          MathUpgradeable.max(lastMintTimestamp, epochStartTime);
      
      mintable += rate * time;
      
      unchecked { ++i; }
  }

  new code:

  for (uint256 i = startingEpoch; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
  
      uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;
      uint256 epochEndTime = cachedGlobalStartTimestamp + (i + 1) * epochLength;
      
      if (block.timestamp <= epochEndTime) {
          uint256 time = block.timestamp - MathUpgradeable.max(lastMintTimestamp, epochStartTime);
          mintable += rate * time;
          break;
      } else {
          uint256 time = epochEndTime - MathUpgradeable.max(lastMintTimestamp, epochStartTime);
          mintable += rate * time;
      }
      
      unchecked { ++i; }
  }

• Calculate the first iteration out of the loop to avoid using the max function (the last mint time can only effect the first iteration)

  old:

  for (uint256 i = startingEpoch; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
  
      uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;
      uint256 epochEndTime = cachedGlobalStartTimestamp + (i + 1) * epochLength;
      
      uint256 time = MathUpgradeable.min(block.timestamp, epochEndTime) -
          MathUpgradeable.max(lastMintTimestamp, epochStartTime);
      
      mintable += rate * time;
      
      unchecked { ++i; }
  }

  new:

  mintable += rate * (MathUpgradeable.min(block.timestamp, epochEndTime) - lastMintTimestamp);
  
  for (uint256 i = startingEpoch + 1; i <= endingEpoch; /** See below ++i */) {
      uint256 rate = epochRate[i];
  
      uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;
      uint256 epochEndTime = cachedGlobalStartTimestamp + (i + 1) * epochLength;
      
      uint256 time = MathUpgradeable.min(block.timestamp, epochEndTime) - epochStartTime;
      
      mintable += rate * time;
      
      unchecked { ++i; }
  }