Badger Citadel contest - ellahi's results

Low

1. Unspecific Compiler Version Pragma

Impact

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

Proof of Concept

  src/CitadelToken.sol::2 => pragma solidity ^0.8.0;
  src/GlobalAccessControl.sol::3 => pragma solidity ^0.8.0;

Recommendation

Consider using a specific compiler version, like 0.8.4.

2. Do not use Deprecated Library Functions

Impact

safeApprove and _setupRole are deprecated.

Proof of Concept

  src/CitadelMinter.sol::133 => IERC20Upgradeable(_citadelToken).safeApprove(_xCitadel, type(uint256).max);
  src/CitadelMinter.sol::136 => IERC20Upgradeable(_xCitadel).safeApprove(_xCitadelLocker, type(uint256).max);
  src/Funding.sol::142 => IERC20(_citadel).safeApprove(address(_xCitadel), type(uint256).max);
  src/GlobalAccessControl.sol::69 => _setupRole(DEFAULT_ADMIN_ROLE, _initialContractGovernance);
  src/GlobalAccessControl.sol::71 => _setupRole(CONTRACT_GOVERNANCE_ROLE, _initialContractGovernance);

Recommendation

Use safeIncreaseAllowance || safeDecreaseAllowance && _grantRole instead.

3. Lack of Zero Address Check

Impact

The functions claim(),setDiscountManager() and setGuestList() take an address as a parameter, but the functions lack a zero address check.

Proof of Concept

StakedCitadelVester.sol#L85, Funding.sol#L373, KnightingRound.sol#L367, StakedCitadel.sol#L329, StakedCitadel.sol#L341

Recommendation

Add a zero address check.

4. Incorrect comment

Impact

Comment states that transferFromDisabled is set to true in initialize. However, it is not set to true anywhere.

Proof of Concept

GlobalAccessControl.sol#L51

bool public transferFromDisabled; // Set to true in initialize

Recommendation

Make changes to satisfy comment, or remove comment.

5. Lacking zero checks.

Impact

Deposit functions are lacking a zero check on _amount parameter.

Proof of Concept

StakedCitadel.sol#L309, StakedCitadel.sol#L319, StakedCitadel.sol#L329, StakedCitadel.sol#L341

Recommendation

Add zero checks.

Tools used

Manual, c4udit.

Gas

1. Don't initialize variables with default value.

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  src/CitadelMinter.sol::152 => for (uint256 i = 0; i < numPools; i++) {
  src/CitadelMinter.sol::180 => uint256 lockingAmount = 0;
  src/CitadelMinter.sol::181 => uint256 stakingAmount = 0;
  src/CitadelMinter.sol::182 => uint256 fundingAmount = 0;
  src/SupplySchedule.sol::103 => uint256 mintable = 0;
  src/SupplySchedule.sol::192 => uint256 mintable = 0;

Recommendation

Remove explicit zero initialization.

2. Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers.

Proof of Concept

  src/CitadelMinter.sol::270 => } else if (_weight > 0) {
  src/CitadelMinter.sol::343 => require(length > 0, "CitadelMinter: no funding pools");
  src/Funding.sol::170 => require(_assetAmountIn > 0, "_assetAmountIn must not be 0");
  src/Funding.sol::209 => if (funding.discount > 0) {
  src/Funding.sol::322 => require(amount > 0, "nothing to sweep");
  src/Funding.sol::340 => require(amount > 0, "nothing to claim");
  src/Funding.sol::424 => require(_citadelPriceInAsset > 0, "citadel price must not be zero");
  src/Funding.sol::452 => require(_citadelPriceInAsset > 0, "citadel price must not be zero");
  src/KnightingRound.sol::125 => _saleDuration > 0,
  src/KnightingRound.sol::129 => _tokenOutPrice > 0,
  src/KnightingRound.sol::172 => require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
  src/KnightingRound.sol::184 => if (boughtAmountTillNow > 0) {
  src/KnightingRound.sol::215 => require(tokenOutAmount_ > 0, "nothing to claim");
  src/KnightingRound.sol::313 => _saleDuration > 0,
  src/KnightingRound.sol::332 => _tokenOutPrice > 0,
  src/KnightingRound.sol::411 => require(amount > 0, "nothing to sweep");
  src/StakedCitadel.sol::835 => if(_fee > 0) {
  src/StakedCitadel.sol::908 => uint256 management_fee = managementFee > 0
  src/StakedCitadelVester.sol::138 => require(_amount > 0, "StakedCitadelVester: cannot vest 0");
  src/SupplySchedule.sol::61 => globalStartTimestamp > 0,
  src/SupplySchedule.sol::91 => cachedGlobalStartTimestamp > 0,
  src/SupplySchedule.sol::180 => globalStartTimestamp > 0,

Recommendation

Use != 0 instead of > 0.

3. Long Revert Strings

Impact

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

  src/CitadelMinter.sol::301 => "CitadelMinter: Sum of propvalues must be 10000 bps"
  src/CitadelMinter.sol::321 => "CitadelMinter: last mint timestamp already initialized"
  src/CitadelMinter.sol::328 => "CitadelMinter: supply schedule start not initialized"
  src/CitadelMinter.sol::370 => "CitadelMinter: funding pool does not exist for removal"
  src/Funding.sol::148 => "Funding: citadel price from oracle flagged and pending review"
  src/Funding.sol::298 => "cannot decrease cap below global sum of assets in"
  src/Funding.sol::325 => "cannot sweep funding asset, use claimAssetToTreasury()"
  src/Funding.sol::390 => "Funding: sale recipient should not be zero"
  src/GlobalAccessControl.sol::118 => "Role string and role do not match"
  src/KnightingRound.sol::122 => "KnightingRound: start date may not be in the past"
  src/KnightingRound.sol::126 => "KnightingRound: the sale duration must not be zero"
  src/KnightingRound.sol::130 => "KnightingRound: the price must not be zero"
  src/KnightingRound.sol::134 => "KnightingRound: sale recipient should not be zero"
  src/KnightingRound.sol::273 => require(!finalized, "KnightingRound: already finalized");
  src/KnightingRound.sol::277 => "KnightingRound: not enough balance"
  src/KnightingRound.sol::295 => "KnightingRound: start date may not be in the past"
  src/KnightingRound.sol::297 => require(!finalized, "KnightingRound: already finalized");
  src/KnightingRound.sol::314 => "KnightingRound: the sale duration must not be zero"
  src/KnightingRound.sol::316 => require(!finalized, "KnightingRound: already finalized");
  src/KnightingRound.sol::333 => "KnightingRound: the price must not be zero"
  src/KnightingRound.sol::351 => "KnightingRound: sale recipient should not be zero"
  src/KnightingRound.sol::384 => require(!finalized, "KnightingRound: already finalized");
  src/StakedCitadel.sol::192 => "performanceFeeGovernance too high"
  src/StakedCitadel.sol::196 => "performanceFeeStrategist too high"
  src/StakedCitadel.sol::508 => "Please withdrawToVault before changing strat"
  src/StakedCitadel.sol::537 => "performanceFeeStrategist too high"
  src/StakedCitadel.sol::632 => "Excessive strategist performance fee"
  src/StakedCitadel.sol::652 => "Excessive governance performance fee"
  src/StakedCitadelVester.sol::137 => require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
  src/StakedCitadelVester.sol::138 => require(_amount > 0, "StakedCitadelVester: cannot vest 0");
  src/SupplySchedule.sol::62 => "SupplySchedule: minting not started"
  src/SupplySchedule.sol::92 => "SupplySchedule: minting not started"
  src/SupplySchedule.sol::96 => "SupplySchedule: already minted up to current block"
  src/SupplySchedule.sol::139 => "SupplySchedule: minting already started"
  src/SupplySchedule.sol::143 => "SupplySchedule: minting must start at or after current time"
  src/SupplySchedule.sol::157 => "SupplySchedule: rate already set for given epoch"
  src/SupplySchedule.sol::181 => "SupplySchedule: minting not started"
  src/SupplySchedule.sol::185 => "SupplySchedule: attempting to mint before start block"
  src/SupplySchedule.sol::189 => "SupplySchedule: already minted up to current block"
  src/SupplySchedule.sol::227 => "total mintable after this iteration",

Recommendation

Consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec. Or shorten the strings to be <= 32 bytes.

4. Use modifiers instead of functions for access control.

Impact

Using functions as modifiers is more expensive then using modifiers themselves.

Proof of Concept

SettAccessControl.sol, StakedCitadel.sol#L258

Example

Bad

    function _onlyOwner() internal view {
        require(msg.sender == owner);
    }

    function bad(uint256 amount) external {
        _onlyOwner(); // 28837 gas
        a = amount;
    }

Good

    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }

    function good(uint256 amount) external onlyOwner {
        a = amount; // 28769 gas
    }

Recommendation

Use actual modifiers to save gas.

Tools used

Manual, c4udit.