Gas Optimizations

Use a more recent version of solidity

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

File: external/MedianOracle.sol (line 1)
```
pragma solidity 0.4.24;
```
File: external/StakedCitadelLocker.sol (line 2)
```
pragma solidity 0.6.12;
```

Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

File: src/CitadelToken.sol (line 2)
```
pragma solidity ^0.8.0;
```
File: src/GlobalAccessControl.sol (line 3)
```
pragma solidity ^0.8.0;
```
File: src/lib/SafeERC20.sol (line 4)
```
pragma solidity ^0.8.0;
```

Unused state variables should be removed

These unused state variables are wasting a lot of gas. The compiler automatically creates non-payable getter functions which is expensive and each variable, since it is given a value, costs a ton of gas to initialize. Variables initialized to zero cost G~sreset~ (2900 gas) and variables initialized to non-zero cost G~sset~ (20000 gas)

File: src/StakedCitadelLocker.sol (lines 91-99)

// ========== Not used ==========
//boost
address public boostPayment = address(0);
uint256 public maximumBoostPayment = 0;
uint256 public boostRate = 10000;
uint256 public nextMaximumBoostPayment = 0;
uint256 public nextBoostRate = 10000;
uint256 public constant denominator = 10000;
// ==============================

Changing from `immutable` to just `private` wastes a lot of gas

One of the changes made after branching this file was to remove the immutable keyword. Doing this changes the variable from a deployment-time replacement to a state variable that gets assigned, costing G~sset~ (20000 gas). The variable's value does not change after initialization, so it should remain immutable

File: src/StakedCitadelLocker.sol (line 117)
```
uint8 private _decimals;
```

Multiple `address` mappings can be combined into a single `mapping` of an `address` to a `struct`, where appropriate

Saves a storage slot for the mapping as well as a non-payable getter for the mapping. Depending on the circumstances and sizes of types, can avoid a G~sset~ (20000 gas). Reads and subsequent writes are also cheaper

File: external/StakedCitadelLocker.sol (lines 76-80)

// user -> reward token -> amount
mapping(address => mapping(address => uint256)) public userRewardPerTokenPaid;
mapping(address => mapping(address => uint256)) public rewards;

File: external/StakedCitadelLocker.sol (lines 88-89)

mapping(address => Balances) public balances;
mapping(address => LockedBalance[]) public userLocks;

File: src/KnightingRound.sol (lines 47-51)

mapping(address => uint256) public boughtAmounts;
/// Whether an account has claimed tokens
/// NOTE: can reset boughtAmounts after a claim to optimize gas
///       but we need to persist boughtAmounts
mapping(address => bool) public hasClaimed;

File: src/StakedCitadel.sol (lines 94-95)

mapping(address => uint256) public additionalTokensEarned;
mapping(address => uint256) public lastAdditionalTokenAmount;

State variables can be packed into fewer storage slots

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate G~sset~ (20000 gas). Reads of the variables are also cheaper

File: external/StakedCitadelLocker.sol (line 63)
```
IERC20Upgradeable public stakingToken; // xCTDL token
```
Variable ordering with two fewer slots: address:rewardTokens, mapping(32):rewardData, mapping(32):rewardDistributors, mapping(32):userRewardPerTokenPaid, mapping(32):rewards, uint256(32):lockedSupply, uint256(32):boostedSupply, user-defined:epochs, mapping(32):balances, mapping(32):userLocks, uint256(32):maximumBoostPayment, uint256(32):boostRate, uint256(32):nextMaximumBoostPayment, uint256(32):nextBoostRate, uint256(32):minimumStake, uint256(32):maximumStake, uint256(32):kickRewardPerEpoch, uint256(32):kickRewardEpochDelay, string(32):_name, string(32):_symbol, user-defined(20):stakingToken, bool(1):isShutdown, uint8(1):_decimals, address(20):boostPayment, address(20):stakingProxy
File: src/Funding.sol (line 32)
```
IERC20 public citadel; /// token to distribute (in vested xCitadel form)
```
Variable ordering with one fewer slots: uint256(32):citadelPriceInAsset, uint256(32):minCitadelPriceInAsset, uint256(32):maxCitadelPriceInAsset, uint256(32):assetDecimalsNormalizationValue, user-defined(20):citadel, bool(1):citadelPriceFlag, user-defined(20):xCitadel, user-defined(20):asset, address(20):citadelPriceInAssetOracle, address(20):saleRecipient, user-defined(*):funding

Pre-calculate repeatedly-checked offsets

Reading from storage is expensive, so it saves gas when only one variable has to be read versus multiple. If there is a calculation which requires multiple storage reads, the calculation should be optimized to pre-calculate as much as possible, and store the intermediate result in storage. Replace the saleDuration variable with a private pre-calculated saleEnd timestamp, and reference that rather than checking both saleStart and saleDuration in multiple places. The duration can be calculated by subtracting the start from the end in a view function. Making this change saves a G~coldsload~ (2100 gas) for each call to buy() and saleEnded()

File: src/KnightingRound.sol (lines 168-171)

    require(
        block.timestamp < saleStart + saleDuration,
        "KnightingRound: already ended"
    );

File: src/KnightingRound.sol (lines 259-261)

    hasEnded_ =
        (block.timestamp >= saleStart + saleDuration) ||
        (totalTokenIn >= tokenInLimit);

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. With each of these individually, caching will replace a G~warmaccess~ (100 gas) with a much cheaper stack read. Less obvious optimizations flagged include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.

File: external/MedianOracle.sol (line 144)

    providerReports[providerAddress][0].timestamp=1;

File: external/MedianOracle.sol (line 173)

            uint256 reportTimestampPast = providerReports[providerAddress][index_past].timestamp;

File: external/MedianOracle.sol (line 213)

    providerReports[provider][0].timestamp = 1;

File: external/StakedCitadelLocker.sol (line 770)

        stakingToken.safeTransfer(stakingProxy, increase);

File: external/StakedCitadelLocker.sol (line 166)

    rewardData[_rewardsToken].lastUpdateTime = uint40(block.timestamp);

File: external/StakedCitadelLocker.sol (line 229)

    uint256(rewardData[_rewardsToken].rewardPerTokenStored).add(

File: external/StakedCitadelLocker.sol (line 781)

            rewards[_account][_rewardsToken] = 0;

File: external/StakedCitadelLocker.sol (line 232)

            rewardData[_rewardsToken].rewardRate).mul(1e18).div(rewardData[_rewardsToken].useBoost ? boostedSupply : lockedSupply)

File: external/StakedCitadelLocker.sol (line 292)
```
    amount = balances[_user].boosted;
```

File: external/StakedCitadelLocker.sol (line 559)

    if (idx == 0 || userLocks[_account][idx - 1].unlockTime < unlockTime) {

File: external/StakedCitadelLocker.sol (line 537)

    uint256 boostRatio = boostRate.mul(_spendRatio).div(maximumBoostPayment==0?1:maximumBoostPayment);

File: external/StakedCitadelLocker.sol (line 509)

            maximumBoostPayment = nextMaximumBoostPayment;

File: external/StakedCitadelLocker.sol (line 506)
```
            boostRate = nextBoostRate;
```

File: external/StakedCitadelLocker.sol (line 762)

    uint256 min = MathUpgradeable.min(minimumStake, minimumStake - _offset);

File: external/StakedCitadelLocker.sol (line 761)

    uint256 max = maximumStake.add(_offset);

File: external/StakedCitadelLocker.sol (line 766)

        IStakingProxy(stakingProxy).withdraw(remove);

File: src/CitadelMinter.sol (line 217)

        IERC20Upgradeable(address(citadelToken)).safeTransfer(address(cachedXCitadel), stakingAmount);

File: src/CitadelMinter.sol (line 276)

        uint256 _newTotalWeight = totalFundingPoolWeight;

File: src/Funding.sol (line 134)

    assetDecimalsNormalizationValue = 10**asset.decimals();

File: src/Funding.sol (line 341)

    asset.safeTransfer(saleRecipient, amount);

File: src/Funding.sol (line 434)
```
            minCitadelPriceInAsset,
```
File: src/Funding.sol (line 461)
```
            minCitadelPriceInAsset,
```
File: src/Funding.sol (line 435)
```
            maxCitadelPriceInAsset
```
File: src/Funding.sol (line 462)
```
            maxCitadelPriceInAsset
```

File: src/KnightingRound.sol (line 148)

    tokenInNormalizationValue = 10**tokenIn.decimals();

File: src/KnightingRound.sol (line 169)

        block.timestamp < saleStart + saleDuration,

File: src/KnightingRound.sol (line 198)

    totalTokenIn = totalTokenIn + _tokenInAmount;

File: src/KnightingRound.sol (line 250)

        limitLeft_ = tokenInLimit - totalTokenIn;

File: src/KnightingRound.sol (line 250)

        limitLeft_ = tokenInLimit - totalTokenIn;

File: src/KnightingRound.sol (line 179)

        require(guestlist.authorized(msg.sender, _proof), "not authorized");

File: src/StakedCitadel.sol (line 774)

    token.safeTransferFrom(msg.sender, address(this), _amount);

File: src/StakedCitadel.sol (line 819)

        uint256 _after = token.balanceOf(address(this));

File: src/StakedCitadel.sol (line 795)

            guestList.authorized(_recipient, _amount, proof),

File: src/StakedCitadel.sol (line 507)

            IStrategy(strategy).balanceOf() == 0,

File: src/StakedCitadel.sol (line 723)
```
    IStrategy(strategy).earn();
```

File: src/StakedCitadel.sol (line 831)

    token.safeTransfer(vesting, _amount);

File: src/StakedCitadel.sol (line 909)

        ? (managementFee * (balance() - _harvestedAmount) * duration) /

File: src/SupplySchedule.sol (line 64)

    return (_timestamp - globalStartTimestamp) / epochLength;

File: src/SupplySchedule.sol (line 184)

        lastMintTimestamp > globalStartTimestamp,

Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()`

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

File: src/lib/SafeERC20.sol (line 79)

        uint256 newAllowance = oldAllowance - value;

Cheaper checks should be done first

Checking of equality to account is cheaper than looking up the gac role via static call, so that check should be on the left of the condition to shortcut the logic

File: src/lib/GlobalAccessControlManaged.sol (line 63)

        gac.hasRole(role, msg.sender) || msg.sender == account,

`.length` should not be looked up in every loop of a `for`-loop

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length. Storage array length checks incur an extra G~warmaccess~ (100 gas) PER-LOOP.

File: external/MedianOracle.sol (line 226)

    for (uint256 i = 0; i < providers.length; i++) {

File: external/StakedCitadelLocker.sol (line 267)

    for (uint256 i = 0; i < userRewards.length; i++) {

File: external/StakedCitadelLocker.sol (line 459)

    for (uint i = nextUnlockIndex; i < locks.length; i++) {

File: external/StakedCitadelLocker.sol (line 777)

    for (uint i; i < rewardTokens.length; i++) {

File: external/StakedCitadelLocker.sol (line 838)

        for (uint i = 0; i < rewardTokens.length; i++) {

File: src/lib/GlobalAccessControlManaged.sol (line 48)
```
    for (uint256 i = 0; i < roles.length; i++) {
```

`++i`/`i++` should be `unchecked{++i}`/`unchecked{++i}` when it is not possible for them to overflow, as is the case when used in `for`- and `while`-loops

File: external/MedianOracle.sol (line 164)

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

File: external/MedianOracle.sol (line 226)

    for (uint256 i = 0; i < providers.length; i++) {

File: external/StakedCitadelLocker.sol (line 267)

    for (uint256 i = 0; i < userRewards.length; i++) {

File: external/StakedCitadelLocker.sol (line 296)

    for (uint i = nextUnlockIndex; i < locksLength; i++) {

File: external/StakedCitadelLocker.sol (line 325)

    for (uint i = locks.length - 1; i + 1 != 0; i--) {

File: external/StakedCitadelLocker.sol (line 363)

    for (uint i = locks.length - 1; i + 1 != 0; i--) {

File: external/StakedCitadelLocker.sol (line 391)

    for (uint i = epochindex - 1; i + 1 != 0; i--) {

File: external/StakedCitadelLocker.sol (line 409)

    for (uint i = _epoch; i + 1 != 0; i--) {

File: external/StakedCitadelLocker.sol (line 428)
```
    for (uint256 i = 0; i < 128; i++) {
```

File: external/StakedCitadelLocker.sol (line 459)

    for (uint i = nextUnlockIndex; i < locks.length; i++) {

File: external/StakedCitadelLocker.sol (line 659)

        for (uint i = nextUnlockIndex; i < length; i++) {

File: external/StakedCitadelLocker.sol (line 777)

    for (uint i; i < rewardTokens.length; i++) {

File: external/StakedCitadelLocker.sol (line [838](https://github.com/Citadel-DAO/staked-citadel-locker/blob/980088335adf7fdc62aa9a0c2556b37c01605dd4/src/StakedCitadelLocker.sol#L838 ...

code-423n4 / 2022-04-badger-citadel-findings