Badger Citadel contest - TerrierLover's results

uint256 can be used instead of uint at getMintable function in ISupplySchedule interface

Target codebase

getMintable function uses uint, but this can be rewritten into uint256. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/interfaces/citadel/ISupplySchedule.sol#L6

function getMintable(uint lastMintTimestamp) external view returns (uint256);

uint will be converted to uint256, so just using uint256 is better.

The caller of getMintable expects uint256 to be used, so it is reasonable to use uint256 instead of uint. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L175-L177

uint256 cachedLastMintTimestamp = lastMintTimestamp;

uint256 mintable = supplySchedule.getMintable(cachedLastMintTimestamp);

Potential workaround

Simply using uint256 instead of uint.

function getMintable(uint256 lastMintTimestamp) external view returns (uint256);

---

Naming inconsistency at setupVesting function in IVesting interface

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/interfaces/citadel/IVesting.sol#L6-L8

function setupVesting(
    address recipient,
    uint256 _amount,
    uint256 _unlockBegin
) external;

Throughout the codebase, _ is added at the prefixes of the arguments. However, recipient does not have _ in its prefix. To be consistent with other codebase, recipient should be _recipient.

Potential workaround

function setupVesting(
    address _recipient,
    uint256 _amount,
    uint256 _unlockBegin
) external;

---

address() is not needed at setFundingPoolWeight function in CitadelMinter.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L257

require(
    address(_pool) != address(0),
    "CitadelMinter: address(0) check"
);

It uses address() function to convert _pool to address, but _pool is already an address. It does not need to use address() at all.

Potential workaround

Simply remove address function.

require(
    _pool != address(0),
    "CitadelMinter: address(0) check"
);

---

Arguments of at mint function have inconsistent naming in CitadelToken.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelToken.sol#L40

function mint(address dest, uint256 amount)

In CitadelToken.sol, the arguments of the functions have _ at their prefixes.

Potential workaround

Add _ at their prefixes.

function mint(address _dest, uint256 _amount)

---

ABIEncoderV2 is not experimental anymore

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L3-L4

pragma solidity 0.8.12;
pragma experimental ABIEncoderV2;

See the solidity document. https://docs.soliditylang.org/en/v0.8.12/layout-of-source-files.html?highlight=ABIEncoderV2#abiencoderv2

Because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2 (please see above) since Solidity 0.7.4.

Potential workaround

Use following instead of pragma experimental ABIEncoderV2;

pragma solidity 0.8.12;
pragma abicoder v2;

---

Avoid using == false to reduce the deployment gas cost

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L147

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

Instead of checking by == false, simply ! can be used. This can decrease the deployment gas cost.

Potential workaround

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

---

No need to set 0 at minCitadelPriceInAsset varialbe at initialize function in Funding.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L137

minCitadelPriceInAsset = 0;

The default value of minCitadelPriceInAsset is 0. So setting minCitadelPriceInAsset seems not needed.

Potential workaround

Just avoid setting 0 at the above code. This should reduce the gas cost as well.

minCitadelPriceInAsset;

---

citadelAmount_ >= _minCitadelOut can be moved up at deposit function in Funding.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L175-L178

funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;
// Take in asset from user
citadelAmount_ = getAmountOut(_assetAmountIn);
require(citadelAmount_ >= _minCitadelOut, "minCitadelOut");

funding.assetCumulativeFunded is updated before the require(citadelAmount_ >= _minCitadelOut, "..") check. The storage variable update should happen after the require check.

Potential workaround

Simply re-order the codebase.

// Take in asset from user
citadelAmount_ = getAmountOut(_assetAmountIn);
require(citadelAmount_ >= _minCitadelOut, "minCitadelOut");
funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;

---

Use uint256 instead of uint at getStakedCitadelAmountOut in Funding.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L224

uint citadelAmount = getAmountOut(_assetAmountIn);

The return value of getAmountOut function is uint256. Although it does not have any issues, but uint256 should be used for the consistency.

Potential workaround

uint256 citadelAmount = getAmountOut(_assetAmountIn);

---

transfer token first before the event emission at sweep function in Funding.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L328-L329

emit Sweep(_token, amount);
IERC20(_token).safeTransfer(saleRecipient, amount);

It emits event first, and the transfer of the token happens. If there is no special reason to have this order, to be consistent with other codebase in this product, it should transfer token first, and then emit event,

Potential workaround

IERC20(_token).safeTransfer(saleRecipient, amount);
emit Sweep(_token, amount);

---

transfer token first before the event emission at sweep function in KnightingRound.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L413-L414

emit Sweep(_token, amount);
ERC20Upgradeable(_token).safeTransfer(saleRecipient, amount);

It emits event first, and the transfer of the token happens. If there is no special reason to have this order, to be consistent with other codebase in this product, it should transfer token first, and then emit event,

Potential workaround

ERC20Upgradeable(_token).safeTransfer(saleRecipient, amount);
emit Sweep(_token, amount);

---

Target codebase

Can only be called by owner is used in the comments below. However, the allowed roles vary on functions. (In the codebase, CONTRACT_GOVERNANCE_ROLE, TECH_OPERATIONS_ROLE, and TREASURY_OPERATIONS_ROLE are used.)

citadel/2022-04-badger-citadel/src/KnightingRound.sol 269: * @notice Finalize the sale after sale duration. Can only be called by owner 286: * @notice Update the sale start time. Can only be called by owner 305: * @notice Update sale duration. Can only be called by owner 324: * @notice Modify the tokenOut price in. Can only be called by owner 342: * @notice Update the tokenIn receipient address. Can only be called by owner 364: * @notice Update the guestlist address. Can only be called by owner 377: * @notice Modify the max tokenIn that this contract can take. Can only be called by owner 396: * @notice Transfers out any tokens accidentally sent to the contract. Can only be called by owner

Potential workaround

If the team thinks that owner is too broad, it should specify the specific roles for each function.

---

Inconsistent naming in StakedCitadel.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L85-L86

string internal constant _defaultNamePrefix = "Staked ";
string internal constant _symbolSymbolPrefix = "x";

They are constant variables, but camel case is used. Other codebase in this project uses capital letters for constant variables, so this seems not consistent.

Potential workaround

string internal constant _DEFAULT_NAME_PREFIX = "Staked ";
string internal constant _SYMBOL_SYMBOL_PREFIX = "x";

---

symbol is used twice at _symbolSymbolPrefix variable in StakedCitadel.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L86

string internal constant _symbolSymbolPrefix = "x";

symbol is used twice in the naming. This seems not necessary.

Potential workaround

Just rename as follows:

string internal constant _symbolPrefix = "x";

---

Inconsistent naming in SupplySchedule.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L25

uint256 public constant epochLength = 21 days;

epochLength is constant variable, but camel case is used. Other codebase in this project uses capital letters for constant variable, so this seems not consistent.

Potential workaround

uint256 public constant EPOCH_LENGTH = 21 days;

---

zero address should be removed at sweepExtraToken function in StakedCitadel.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L698-L700

function sweepExtraToken(address _token) external {
    _onlyGovernanceOrStrategist();
    require(address(token) != _token, "No want");

Currently _token can be zero address. It should check zero address as well.

Potential workaround

function sweepExtraToken(address _token) external {
    _onlyGovernanceOrStrategist();
    require(address(token) != _token, "No want");
    require(_token != address(0), "...");

---

_gac can be zero address at initialize function in StakedCitadelVester.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L59-L65

function initialize(
    address _gac,
    address _vestingToken,
    address _vault
) external initializer {
    require(_vestingToken != address(0), "Address zero invalid");
    require(_vault != address(0), "Address zero invalid");

There are zero address check for _vestingToken and _vault variables, but _gac does not have one. If this is not intentional, it should also check zero address for _gac.

Potential workaround

function initialize(
    address _gac,
    address _vestingToken,
    address _vault
) external initializer {
    require(_gac != address(0), "Address zero invalid");
    require(_vestingToken != address(0), "Address zero invalid");
    require(_vault != address(0), "Address zero invalid");

---

Inconsistent naming at vest function in StakedCitadelVester.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L133

function vest(
    address recipient,
    uint256 _amount,
    uint256 _unlockBegin
) external {

only recipient does not have _ in its prefix. To be consistent with other codebase, it should also have _.

Potential workaround

function vest(
    address _recipient,
    uint256 _amount,
    uint256 _unlockBegin
) external {

---

_globalAccessControl variable can be zero address

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L27-L33

function __GlobalAccessControlManaged_init(address _globalAccessControl)
    public
    onlyInitializing
{
    __Pausable_init_unchained();
    gac = IGac(_globalAccessControl);
}

There is not zero address check for _globalAccessControl variable.

Potential workaround

It should do zero address check.

function __GlobalAccessControlManaged_init(address _globalAccessControl)
    public
    onlyInitializing
{
    require(_globalAccessControl != address(0), "...");
    __Pausable_init_unchained();
    gac = IGac(_globalAccessControl);
}

---

Do zero address check at claim function in StakedCitadelVester.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L85

function claim(address recipient, uint256 amount) external {
    uint256 claimable = claimableBalance(msg.sender);

There is no zero address check for recipient argument.

Potential change

Add zero address check for recipient not to cause the unexpected behavior for users.

function claim(address recipient, uint256 amount) external {
    require(recipient != address(0), "...");
    uint256 claimable = claimableBalance(msg.sender);

---

Do zero check for amount early at claim function in StakedCitadelVester.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L90

function claim(address recipient, uint256 amount) external {
    uint256 claimable = claimableBalance(msg.sender);
    if (amount > claimable) {
        amount = claimable;
    }
    if (amount != 0) {
        vesting[msg.sender].claimedAmounts =
            vesting[msg.sender].claimedAmounts +
            amount;
        vestingToken.safeTransfer(recipient, amount);
        emit Claimed(msg.sender, recipient, amount);
    }
}

If amount is 0, users cannot do meaningful operations but simply pay higher gas fee. It can do zero check for amount at the first part of the function.

Potential workaround

function claim(address recipient, uint256 amount) external {
    require(amount != 0, "...");

    uint256 claimable = claimableBalance(msg.sender);
    if (amount > claimable) {
        amount = claimable;
    }

    vesting[msg.sender].claimedAmounts =
        vesting[msg.sender].claimedAmounts +
        amount;
    vestingToken.safeTransfer(recipient, amount);
    emit Claimed(msg.sender, recipient, amount);
}

---

setDiscountLimits can potentially block the user operation if _maxDiscount is set lower than _minDiscount

Target codebase

setDiscountLimits function sets _minDiscount and _maxDiscount.

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L362-L363

funding.minDiscount = _minDiscount;
funding.maxDiscount = _maxDiscount;

Since there is no check to confirm that _minDiscount < _maxDiscount is true, if _maxDiscount variable has value which is lower than _minDiscount, it potentially blocks the user operation.

Potential workaround

Simply setting the additional condition can be helpful

require(_minDiscount < _maxDiscount , "...");

---

Consider adding nonReentrant modifier for the external functions which call other contracts

Target codebase

Here are codebase which contain external functions which call the other contracts.

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L309 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L319 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L329 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L341 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L350 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L363 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L375 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L382 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L85

Since nonReentrant modifier is used at other function, it is worth considering adding nonReentrant modifier for the above mentioned functions.

Potential change

Add nonReentrant modifier for the above mentioned functions

---

Setting 0 at i is not needed in the for loop at getFundingPoolWeights function in CitadelMinter.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152

for (uint256 i = 0; i < numPools; i++) {

i will be initialized with 0 so no need to set 0.

Potential change

Simply avoid setting 0 can save small gas cost.

for (uint256 i; i < numPools; i++) {

---

pool variable do not need to be defined in the for loop at getFundingPoolWeights in CitadelMinter.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L153-L157

for (uint256 i = 0; i < numPools; i++) {
    address pool = fundingPools.at(i);
    uint256 weight = fundingPoolWeights[pool];

    pools[i] = pool;
    weights[i] = weight;
}

weight varialbe is not used multiple times, so it can omit declaring them.

Potential change

Simply omitting declaring weight variable can save gas cost.

for (uint256 i = 0; i < numPools; i++) {
    address pool = fundingPools.at(i);

    pools[i] = pool;
    weights[i] = fundingPoolWeights[pool];
}

---

lockingAmount, stakingAmount and fundingAmount variables do not need to be declared at mintAndDistribute in CitadelMinter.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L180-L182

uint256 lockingAmount = 0;
uint256 stakingAmount = 0;
uint256 fundingAmount = 0;

No need to set 0 at uint256 variable since as a default uint256 variable becomes 0.

Potential change

Avoiding declaring these variables can save some gas cost.

uint256 lockingAmount;
uint256 stakingAmount;
uint256 fundingAmount;

---

Use != 0 instead of > 0 at setFundingPoolWeight in CitadelMinter.sol to reduce deployment gas cost

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L270

if (_weight == 0 && poolExists) {
    _removeFundingPool(_pool);

    emit FundingPoolWeightSet(_pool, _weight, totalFundingPoolWeight);
} else if (_weight > 0) {

It can use != 0 instead of > 0 to reduce the gas cost.

Potential change

if (_weight == 0 && poolExists) {
    _removeFundingPool(_pool);

    emit FundingPoolWeightSet(_pool, _weight, totalFundingPoolWeight);
} else if (_weight != 0) {

Gas improvements

The deployment gas cost of CitadelMinter contract can be reduced.

---

Reduce gas cost by simplifying the weight calculation at setFundingPoolWeight in CitadelMinter.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L276-L280

uint256 _newTotalWeight = totalFundingPoolWeight;
_newTotalWeight = _newTotalWeight - fundingPoolWeights[_pool];
fundingPoolWeights[_pool] = _weight;
_newTotalWeight = _newTotalWeight + _weight;
totalFundingPoolWeight = _newTotalWeight;

Potential change

Simply avoiding the numbers of setting values at variables can reduce the gast cost.

uint256 _newTotalWeight = totalFundingPoolWeight - fundingPoolWeights[_pool] + _weight;
fundingPoolWeights[_pool] = _weight;
totalFundingPoolWeight = _newTotalWeight;

Gas improvements

The gas cost of testSetFundingPoolWeight() decreases with this change from 247334 to 247300

---

Use != 0 instead of > 0 at _transferToFundingPools in CitadelMinter.sol to reduce deployment gas cost

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L343

require(length > 0, "CitadelMinter: no funding pools");

It can use != 0 instead of > 0 to reduce the gas cost.

Potential change

require(length != 0, "CitadelMinter: no funding pools");

---

_addFundingPool function can be removed to reduce the gas cost

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L374

function _addFundingPool(address _pool) internal {
    require(
        fundingPools.add(_pool),
        "CitadelMinter: funding pool already exists"
    );
}

_addFundingPool function is only used by 1 place (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L273-L275). In this case, _addFundingPool does not need to be defined and it simply can put the logic instead of calling the function. This simplification can reduce gas cost as well.

Potential change

Remove _addFundingPool function and port the logic to line 274 of CitadelMinter.sol

if (!poolExists) {
    require(
        fundingPools.add(_pool),
        "CitadelMinter: funding pool already exists"
    );
}

Gas improvement

The gas cost of testSetFundingPoolWeight() decreases with this change from 247334 to 247300

---

Refactor of getMintable function in SupplySchedule.sol can reduce the deployment gas cost

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L112-L120

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;

There are following refactoring opportunities:

• rate variable does not need to be defined
• The calculation to get epochEndTime variable can use epochStartTime variable
• epochEndTime does not need to be defined

Potential change

uint256 epochStartTime = cachedGlobalStartTimestamp + i * epochLength;

uint256 time = MathUpgradeable.min(block.timestamp, epochStartTime + epochLength) -
    MathUpgradeable.max(lastMintTimestamp, epochStartTime);

mintable += epochRate[i] * time;

---

onlyRoles modifier is not used throughout the codebase

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L46

// @dev only holders of any of the given set of roles on the GAC can call
modifier onlyRoles(bytes32[] memory roles) {
    bool validRoleFound = false;
    for (uint256 i = 0; i < roles.length; i++) {
        bytes32 role = roles[i];
        if (gac.hasRole(role, msg.sender)) {
            validRoleFound = true;
            break;
        }
    }
    require(validRoleFound, "GAC: invalid-caller-role");
    _;
}

Potential change

onlyRoles can be removed.

---

Change the order of external contract call in onlyRoleOrAddress modifier in GlobalAccessControlManaged.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L61-L67

modifier onlyRoleOrAddress(bytes32 role, address account) {
    require(
        gac.hasRole(role, msg.sender) || msg.sender == account,
        "GAC: invalid-caller-role-or-address"
    );
    _;
}

gac.hasRole(...) requires the call to the external contract. Hence, msg.sender == account check should happen first.

Potential change

modifier onlyRoleOrAddress(bytes32 role, address account) {
    require(
        msg.sender == account || gac.hasRole(role, msg.sender),
        "GAC: invalid-caller-role-or-address"
    );
    _;
}

---

Change the order of external contract call in gacPausable modifier in GlobalAccessControlManaged.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L71-L72

modifier gacPausable() {
    require(!gac.paused(), "global-paused");
    require(!paused(), "local-paused");
    _;
}

gac.paused() requires the call to the external contract. Hence, !paused() check should happen first.

Potential change

modifier gacPausable() {
    require(!paused(), "local-paused");
    require(!gac.paused(), "global-paused");
    _;
}

---

No need to set 0 at mintable variable at getMintable function in

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L103

uint256 mintable = 0;

Not setting 0 at mintable variable can reduce the gas cost.

Potential workaround

uint256 mintable;

---

unchecked can be used to reduce the gas cost at getRemainingFundable in Funding.sol

Target codebase

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L236

if (assetCumulativeFunded < assetCap) {
    limitLeft_ = assetCap - assetCumulativeFunded;
}

assetCap - assetCumulativeFunded will not be underflown. So it can be wrapped with unchecked.

Potential workaround

if (assetCumulativeFunded < assetCap) {
    unchecked {
        limitLeft_ = assetCap - assetCumulativeFunded;
    }
}

---