search

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

0 stars 1 forks source link

Gas Optimizations #221

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Gas Report

Table of Contents

Caching storage variables in memory to save gas

PROBLEM

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

scope: mintAndDistribute()

CitadelMinter.sol:178
CitadelMinter.sol:217

Funding.sol

scope: claimAssetToTreasury()

    Funding.sol:339:
    Funding.sol:341:
    Funding.sol:343:

scope: updateCitadelPriceInAsset()

    Funding.sol:428
    Funding.sol:434
    Funding.sol:429
    Funding.sol:435

KnightingRound.sol

scope: buy()

    KnightingRound.sol:167
    KnightingRound.sol:169
        KnightingRound.sol:174
        KnightingRound.sol:198
    KnightingRound.sol:178
    KnightingRound.sol:179

StakedCitadel.sol

scope: depositFor()

        StakedCitadel.sol:773
        StakedCitadel.sol:774
        StakedCitadel.sol:775

scope: _withdraw()

    StakedCitadel.sol:815
    StakedCitadel.sol:819
    StakedCitadel.sol:831

scope: _depositForWithAuthorization()

    StakedCitadel.sol:793
    StakedCitadel.sol:795

scope: setStrategy()

    StakedCitadel.sol:505
    StakedCitadel.sol:507

scope: earn()

    StakedCitadel.sol:722
    StakedCitadel.sol:723

scope: withdraw()

    StakedCitadel.sol:830
    StakedCitadel.sol:831

SupplySchedule.sol

scope: getMintableDebug()

    SupplySchedule.sol:180
    SupplySchedule.sol:184
    SupplySchedule.sol:197
    SupplySchedule.sol:200
    SupplySchedule.sol:204
    SupplySchedule.sol:211
    SupplySchedule.sol:212

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

CitadelToken.sol

scope: initialize()

CitadelToken.sol:22: string memory _name, string memory _symbol

GlobalAccessControl.sol

scope: initializeNewRole()

GlobalAccessControl.sol:107: string memory roleString

StakedCitadel.sol

scope: initialize()

StakedCitadel.sol:167: string memory _name, string memory _symbol, uint256[4] memory _feeConfig

scope: deposit()

StakedCitadel.sol:319: bytes32[] memory proof

scope: depositAll()

StakedCitadel.sol:341: bytes32[] memory proof

scope: depositFor()

StakedCitadel.sol:363: bytes32[] memory proof

scope: _depositWithAuthorization()

StakedCitadel.sol:780: bytes32[] memory proof

scope: _depositForWithAuthorization()

StakedCitadel.sol:788: bytes32[] memory proof

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparisons with zero for unsigned integers

PROBLEM

>0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

scope: _transferToFundingPools()

    CitadelMinter.sol:343

Funding.sol

scope: deposit()

    Funding.sol:170

scope: sweep()

    Funding.sol:322

scope: claimAssetToTreasury()

    Funding.sol:340

scope: updateCitadelPriceInAsset()

    Funding.sol:424

scope: updateCitadelPriceInAsset()

    Funding.sol:452

KnightingRound.sol

scope: function initialize()

    KnightingRound.sol:125
    KnightingRound.sol:129

scope: buy()

    KnightingRound.sol:172

scope: claim()

    KnightingRound.sol:215

scope: setSaleDuration()

    KnightingRound.sol:313

scope: setTokenOutPrice()

    KnightingRound.sol:332

scope: sweep()

    KnightingRound.sol:411

StakedCitadelVester.sol

scope: vest()

    StakedCitadelVester.sol:138

SupplySchedule.sol

scope: getEpochAtTimestamp()

    SupplySchedule.sol:61

scope: getMintable()

    SupplySchedule.sol:91

scope: getMintableDebug()

    SupplySchedule.sol:180

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with !0

Comparison Operators

PROBLEM

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

    CitadelMinter.sol:272

Funding.sol

    Funding.sol:172
    Funding.sol:178
    Funding.sol:270
    Funding.sol:271

KnightingRound.sol

    KnightingRound.sol:120
    KnightingRound.sol:167
    KnightingRound.sol:173
    KnightingRound.sol:259
    KnightingRound.sol:275
    KnightingRound.sol:293

StakedCitadel.sol

    StakedCitadel.sol:190
    StakedCitadel.sol:194
    StakedCitadel.sol:198
    StakedCitadel.sol:202
    StakedCitadel.sol:523
    StakedCitadel.sol:535
    StakedCitadel.sol:550
    StakedCitadel.sol:588
    StakedCitadel.sol:613
    StakedCitadel.sol:630
    StakedCitadel.sol:651
    StakedCitadel.sol:666

StakedCitadelVester.sol

    StakedCitadelVester:111

SupplySchedule.sol

    SupplySchedule.sol:111
    SupplySchedule.sol:142
    SupplySchedule.sol:208

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-require(citadelAmount_ >= _minCitadelOut, "minCitadelOut");
+require(citadelAmount_ > _minCitadelOut - 1, "minCitadelOut");

When the comparison is with a constant storage variable, you can also do the increment in the storage variable declaration

example:

-require(
            _feeConfig[1] <= PERFORMANCE_FEE_HARD_CAP,
            "performanceFeeStrategist too high"
        );
+require(
            _feeConfig[1] < PERFORMANCE_FEE_HARD_CAP_PLUS_ONE,
            "performanceFeeStrategist too high"
        );

and

-uint256 public constant PERFORMANCE_FEE_HARD_CAP = 3_000;
+uint256 public constant PERFORMANCE_FEE_HARD_CAP_PLUS_ONE = 3_001;

Custom Errors

PROBLEM

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

    CitadelMinter.sol:116
    CitadelMinter.sol:256
    CitadelMinter.sol:272
    CitadelMinter.sol:299
    CitadelMinter.sol:319
    CitadelMinter.sol:326
    CitadelMinter.sol:343
    CitadelMinter.sol:368
    CitadelMinter.sol:374

Funding.sol

    Funding.sol:113
    Funding.sol:117
    Funding.sol:170
    Funding.sol:171
    Funding.sol:178
    Funding.sol:270
    Funding.sol:271
    Funding.sol:296
    Funding.sol:322
    Funding.sol:323
    Funding.sol:340
    Funding.sol:361
    Funding.sol:388
    Funding.sol:424
    Funding.sol:425
    Funding.sol:452

GlobalAccessControl.sol

    GlobalAccessControl.sol:95
    GlobalAccessControl.sol:100
    GlobalAccessControl.sol:112
    GlobalAccessControl.sol:116

KnightingRound.sol

    KnightingRound.sol:120
    KnightingRound.sol:124
    KnightingRound.sol:128
    KnightingRound.sol:132
    KnightingRound.sol:167
    KnightingRound.sol:168
    KnightingRound.sol:172
    KnightingRound.sol:173
    KnightingRound.sol:179
    KnightingRound.sol:185
    KnightingRound.sol:210
    KnightingRound.sol:211
    KnightingRound.sol:215
    KnightingRound.sol:273
    KnightingRound.sol:274
    KnightingRound.sol:275
    KnightingRound.sol:293
    KnightingRound.sol:297
    KnightingRound.sol:312
    KnightingRound.sol:316
    KnightingRound.sol:331
    KnightingRound.sol:349
    KnightingRound.sol:384
    KnightingRound.sol:411

StakedCitadel.sol

    StakedCitadel.sol:180
    StakedCitadel.sol:181
    StakedCitadel.sol:182
    StakedCitadel.sol:183
    StakedCitadel.sol:184
    StakedCitadel.sol:185
    StakedCitadel.sol:186
    StakedCitadel.sol:187
    StakedCitadel.sol:190
    StakedCitadel.sol:194
    StakedCitadel.sol:198
    StakedCitadel.sol:202
    StakedCitadel.sol:262
    StakedCitadel.sol:270
    StakedCitadel.sol:441
    StakedCitadel.sol:487
    StakedCitadel.sol:502
    StakedCitadel.sol:506
    StakedCitadel.sol:523
    StakedCitadel.sol:535
    StakedCitadel.sol:550
    StakedCitadel.sol:562
    StakedCitadel.sol:574
    StakedCitadel.sol:588
    StakedCitadel.sol:613
    StakedCitadel.sol:630
    StakedCitadel.sol:650
    StakedCitadel.sol:666
    StakedCitadel.sol:700
    StakedCitadel.sol:718
    StakedCitadel.sol:768
    StakedCitadel.sol:769
    StakedCitadel.sol:770
    StakedCitadel.sol:794
    StakedCitadel.sol:809

StakedCitadelVester.sol

    StakedCitadelVester.sol:64
    StakedCitadelVester.sol:65
    StakedCitadelVester.sol:137
    StakedCitadelVester.sol:138

SupplySchedule.sol

    SupplySchedule.sol:60
    SupplySchedule.sol:90
    SupplySchedule.sol:94
    SupplySchedule.sol:137
    SupplySchedule.sol:141
    SupplySchedule.sol:155
    SupplySchedule.sol:179
    SupplySchedule.sol:183
    SupplySchedule.sol:187

TOOLS USED

Manual Analysis

MITIGATION

Replace require statements with custom errors.

Default value initialization

PROBLEM

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

Instances include:

CitadelMinter.sol

CitadelMinter.sol:152: for (uint256 i = 0; i < numPools; i++) {
CitadelMinter.sol:180: uint256 lockingAmount = 0;
CitadelMinter.sol:181: uint256 stakingAmount = 0;
CitadelMinter.sol:182: uint256 fundingAmount = 0;

SupplySchedule.sol

SupplySchedule.sol:103: uint256 mintable = 0;
SupplySchedule.sol:192: uint256 mintable = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol: 343

SupplySchedule.sol

SupplySchedule.sol: 197

TOOLS USED

Manual Analysis

MITIGATION

In both instances, the storage variable is read multiple times in the function and it is recommended to cache them into memory, then passing these cached variables in the emit statement, as explained in the cache paragraph

Inline functions

PROBLEM

When we define internal functions to perform computation:

When it does not affect readability, it is recommended to inline functions in order to save gas

PROOF OF CONCEPT

Instances include:

StakedCitadel.sol

StakedCitadel.sol: 780

TOOLS USED

Manual Analysis

MITIGATION

Inline this function where it is called:

StakedCitadel.sol: 310
StakedCitadel.sol: 323
StakedCitadel.sol: 330
StakedCitadel.sol: 342

Prefix increments

PROBLEM

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol:152
CitadelMinter.sol:344

SupplySchedule.sol

SupplySchedule.sol:208

TOOLS USED

Manual Analysis

MITIGATION

change i++ to ++i

Short-circuiting

PROBLEM

Reading from storage costs 100 gas. When checking conditions using the AND operator, we can save gas by not evaluating the second expression if the first one is false.

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol:266

TOOLS USED

Manual Analysis

MITIGATION

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol:39:
bool public citadelPriceFlag; /// Flag citadel price for review by guardian if it exceeds min and max bounds;

uint256 public assetDecimalsNormalizationValue;

address public citadelPriceInAssetOracle;
address public saleRecipient;

TOOLS USED

Manual Analysis

MITIGATION

Place citadelPriceFlag after SaleRecipient to save one storage slot

uint256 public assetDecimalsNormalizationValue;

address public citadelPriceInAssetOracle;
address public saleRecipient;
+bool public citadelPriceFlag;

Uint256 instead of Uint8

PROBLEM

Each storage slot has 256 bits. Whenever a uint smaller than 256 (or even a bool) is pulled from storage, the EVM casts it to a uint256. Calculations are also unexceptionally performed in uint256 by the EVM.

PROOF OF CONCEPT

Instances include

KnightingRound.sol

KnightingRound.sol:69
KnightingRound.sol:70
KnightingRound.sol:76
KnightingRound.sol:162

TOOLS USED

Manual Analysis

MITIGATION

Replace uint8 with uint256

Unchecked arithmetic

PROBLEM

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol: 152: i bounded by numPools, overflow check unnecessary

CitadelMinter.sol: 344: i bounded by length, overflow check unnecessary

CitadelMinter.sol: 364: totalFundingPoolWeight is greater than or equal to currentPoolWeight, underflow check unnecessary

Funding.sol

Funding.sol: 212: MAX_BPS is less than the maximum discount, underflow check unnecessary

Funding.sol: 236: assetCumulativeFunded is less than assetCap, underflow check unnecessary

KnightingRound.sol

KnightingRound.sol: 198: totalTokenIn + _tokenInAmount is capped by tokenInLimit, overflow check unnecessary

KnightingRound.sol: 250: totalTokenIn is less than tokenInLimit, underflow check unnecessary

StakedCitadel.sol

StakedCitadel.sol: 817: b is less than r, underflow check unnecessary

StakedCitadel.sol: 821: b + diff capped by r (see line 821 and 817), overflow check unnecessary

StakedCitadel.sol: 827: _fee is less than r, underflow check unnecessary

SupplySchedule.sol

SupplySchedule.sol: 208: i is capped by endingEpoch, overflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

liveactionllama commented 2 years ago

Closing this as it appears to be a near-exact duplicate of issue #224