Badger Citadel contest - securerodd's results

BADGER Contest

April 18, 2022

@securerodd

Low Risk Findings

1. xCTDL Vault can Alter Vesting Duration

Within StakedCitadel.sol, the function vest(address recipient, uint256 _amount, uint256 _unlockBegin) allows any value of _unlockBegin to be passed in. This could effectively defeat the purpose of the vestingDuration, as _unlockBegin can be set to something arbitrarily high (such that funds will not vest) or something arbitrarily low (meaning the vestingDuration could be bypassed).

Current Code:

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
        require(_amount > 0, "StakedCitadelVester: cannot vest 0");

        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

Recommendation: Consider calculating the vesting[recipient].unlockEnd based on the time instead of a passed in value. Ex: vesting[recipient].unlockEnd = block.timestamp + vestingDuration;

2. Lack of SupplyCap on Mint in CitadelToken

The CitadelToken.sol does not contain a supplycap for minting.

Code:

    function mint(address dest, uint256 amount)
        external
        onlyRole(CITADEL_MINTER_ROLE)
        gacPausable
    {
        _mint(dest, amount);
    }

CTDL is the base token for the system and as such, not having supply restrictions increases the potential for inflationary issues and/or concerns.

Recommendation: Consider establishing a cap on the supply of CTDL for the system.

BADGER Contest

April 18, 2022

@securerodd

Gas Optimizations

1. Struct Packing

The parameters for the vesting schedule in StakedCitadelVester.sol are stored in the below struct:

    struct VestingParams {
        uint256 unlockBegin;
        uint256 unlockEnd;
        uint256 lockedAmounts;
        uint256 claimedAmounts;
    }

Since the unlockBegin and unlockEnd variables inside of the struct should be based on current time and vesting durations, they are good candidates for struct packing. They could be reduced to uint128 quite easily so that they both fit in the same storage slot and yet they would still allow for ridiculously large times.

Recommended:

    struct VestingParams {
        uint128 unlockBegin;
        uint128 unlockEnd;
        uint256 lockedAmounts;
        uint256 claimedAmounts;
    }

2. For Loop Refactoring

The for loops used throughout the codebase can benefit from one or more of the following gas saving techniques:

1. Storing the array length in a local variable in memory before comparisons
2. Using a prefix increment instead of postfix
3. unchecked increments of i

Example code in GlobalAccessControlManaged.sol:

    modifier onlyRoles(bytes32[] memory roles) {
        bool validRoleFound = false;
        for (uint256 i = 0; i < roles.length; i++) {
            bytes32 role = roles[i];
            ...
        }

Recommended code:

    modifier onlyRoles(bytes32[] memory roles) {
        bool validRoleFound = false;
        uint256 numRoles = roles.length;
        for (uint256 i = 0; i < numRoles;) {
            bytes32 role = roles[i];
            ...
            unchecked {++i};
        }

Example code in getFundingPoolWeights() within CitadelMinter.sol:

        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];
            ...
        }

Recommended code:

        uint256 numPools = fundingPools.length();
        pools = new address[](numPools);
        weights = new uint256[](numPools);

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

Example code in _transferToFundingPools(uint256 _citadelAmount) within CitadelMinter.sol:

        uint256 length = fundingPools.length();
        // Use cached to save 96 gas per loop read
        uint256 cachedTotalFundingPoolWeight = totalFundingPoolWeight;

        require(length > 0, "CitadelMinter: no funding pools");
        for (uint256 i; i < length; ++i) {
            address pool = fundingPools.at(i);
            ...
        }

Recommended code:

        uint256 length = fundingPools.length();
        // Use cached to save 96 gas per loop read
        uint256 cachedTotalFundingPoolWeight = totalFundingPoolWeight;

        require(length > 0, "CitadelMinter: no funding pools");
        for (uint256 i; i < length;) {
            address pool = fundingPools.at(i);
            ...
            unchecked {++i};
        }

3. _before Value Calculated Twice

In the `` function within .sol, the value stored in _pool is calculated by calling balance(). Immediately following, the value stored in _before is calculated using token.balanceOf(address(this)). balance() is a wrapper around token.balanceOf(address(this)) making this set of actions inefficient.

Current Code:

       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);

Recommended Code:

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

4. != 0 is More Efficient than > 0 in Require Statements

within require statements, != comparisons are slightly more gas efficient than > and when a uint is being compared to 0, != is effectively the same as > 0.

Gas savings is typically 3 per statement.

Locations:

• src/CitadelMinter.sol::343 => require(length > 0, "CitadelMinter: no funding pools");
• 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::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::172 => require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
• src/KnightingRound.sol::215 => require(tokenOutAmount_ > 0, "nothing to claim");
• src/KnightingRound.sol::411 => require(amount > 0, "nothing to sweep");
• src/StakedCitadelVester.sol::138 => require(_amount > 0, "StakedCitadelVester: cannot vest 0");
• src/interfaces/convex/BoringMath.sol::20 => require(b > 0, "BoringMath: division by zero");
• src/interfaces/convex/BoringMath.sol::102 => require(b > 0, "BoringMath: division by zero");
• src/interfaces/convex/BoringMath.sol::122 => require(b > 0, "BoringMath: division by zero");
• src/interfaces/convex/BoringMath.sol::142 => require(b > 0, "BoringMath: division by zero");
• src/StakedCitadelVester.sol::138 => require(_amount > 0, "StakedCitadelVester: cannot vest 0");