Platform: Code4rena
Start Date: 14/04/2022
Pot Size: $75,000 USDC
Total HM: 8
Participants: 72
Period: 7 days
Judge: Jack the Pug
Total Solo HM: 2
Id: 110
League: ETH
Rank: 29/72
Findings: 2
Award: $309.78
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, Hawkeye, Jujic, MaratCerby, Picodes, Ruhum, SolidityScan, TerrierLover, TomFrenchBlockchain, TrungOre, VAD37, Yiko, berndartmueller, cmichel, csanuragjain, danb, defsec, delfin454000, dipp, ellahi, fatherOfBlocks, georgypetrov, gs8nrv, gzeon, horsefacts, hubble, hyh, ilan, jah, joestakey, kebabsec, kenta, kyliek, m9800, minhquanym, oyc_109, p_crypt0, peritoflores, rayn, reassor, remora, rfa, robee, scaraven, securerodd, shenwilly, sorrynotsorry, tchkvsky, teryanarmen, z3s
128.6208 USDC - $128.62
When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.
Low
Instances include:
Funding.sol:225: xCitadel.getPricePerFullShare()
StakeCitadel.sol:808: totalSupply() StakeCitadel.sol:890: _pool
Manual Analysis
Before doing these computations, add a non-zero check to the variables aforementioned.
Some external functions calling the ERC20 methods safeTransfer or safeTransferFrom do not have the nonReentrant modifier and are hence unprotected to reentrancy (besides the gas limit on the methods)
Low
Instances include:
Funding.sol: 334 claimAssetToTreasury()
KnightingRound.sol: 209 claim()
StakedCitadel.sol: 698 sweepExtraToken(address _token) StakedCitadel.sol: 717 earn()
StakedCitadelVester.sol: 85 claim(address recipient, uint256 amount)
Manual Analysis
Use the nonReentrant modifier on these functions.
There is no restriction on the numbers of pool added, either in _addFundingPool() or setFundingPoolWeight(). Hence, the for loop in CitadelMinter.sol is unbounded, which could result in the gas required to perform the function call exceeding the gas limit if the number of pools is too high, preventing the distribution of citadel tokens to the funding pools.
Low
Instances include:
CitadelMinter.sol:344:
Manual Analysis
Add an upper limit to the total amount of funding pools, and add a check before adding a pool in setFundingPoolWeight(): 274
There are a few typos in some comments
Non-Critical
Instances include:
CitadelMinter.sol: 101 expection
Funding.sol: 289 cumulatiive
Manual Analysis
Correct the typos.
Some of the function comments are missing function parameters or returns
Non-Critical
Instances include:
Funding.sol:110 address _citadelPriceInAssetOracle
GlobalAccessControl.sol: 107 bytes32 role, string memory roleString,bytes32 adminRole
KnightingRound.sol: 109 address _globalAccessControl KnightingRound.sol: 209 uint256 tokenOutAmount_
StakedCitadel.sol: 175 address _vesting StakedCitadel.sol: 363 bytes32[] memory proof
Manual Analysis
Add a comment for these parameters
Some functions are missing comments.
Non-Critical
Instances include:
CitadelMinter.sol: 143 getFundingPoolWeights() CitadelMinter.sol: 314 initializeLastMintTimestamp() CitadelMinter.sol: 338 _transferToFundingPools(uint256 _citadelAmount) CitadelMinter.sol: 362 _removeFundingPool(address _pool) CitadelMinter.sol: 374 _addFundingPool(address _pool)
Funding.sol: 278 clearCitadelPriceFlag() Funding.sol: 383 setSaleRecipient(address _saleRecipient) Funding.sol: 397 setCitadelAssetPriceBounds(uint256 _minPrice, uint256 _maxPrice)
GlobalAccessControl.sol: 94 pause() GlobalAccessControl.sol: 99 unpause()
KnightingRound.sol: 109 address _globalAccessControl KnightingRound.sol: 209 uint256 tokenOutAmount_
StakedCitadelVester.sol: 59 initialize(address _gac, address _vestingToken, address _vault)
SupplySchedule.sol: 43 initialize(address _gac) SupplySchedule.sol: 67 getCurrentEpoch() SupplySchedule.sol: 71 getEmissionsForEpoch(uint256 _epoch) SupplySchedule.sol: 79 getEmissionsForCurrentEpoch() SupplySchedule.sol: 84 getMintable(uint256 lastMintTimestamp) SupplySchedule.sol: 132 setMintingStart(uint256 _globalStartTimestamp) SupplySchedule.sol: 150 setEpochRate(uint256 _epoch, uint256 _rate)
Manual Analysis
Add comments to these functions
When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.
Non-Critical
Instances include:
KnightingRound.sol: 47 mapping(address => uint256) public boughtAmounts; KnightingRound.sol: 51 mapping(address => bool) public hasClaimed;
StakedCitadel.sol: 94: mapping(address => uint256) public additionalTokensEarned; StakedCitadel.sol: 95: mapping(address => uint256) public lastAdditionalTokenAmount;
Manual Analysis
Group the related data in a struct and use one mapping. For instance, for the StakedCitadel.sol mappings, the mitigation could be:
struct AdditionalTokens { uint256 tokensEarned; uint256 lastAmount; }
And it would be used as a state variable:
mapping(address => AdditionalTokens) additionalTokens;
It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission
Non-Critical
Instances include:
Funding.sol: 328 Sweep(_token, amount) //emitted before amount transferred to saleRecipient
KnightingRound.sol: 413 Sweep(_token, amount) //emitted before amount transferred to saleRecipient
Manual Analysis
Place the event emission in the last position in the function.
Though they are allowed in Solidity and do not pose a direct security issue, it is best practice to avoid declaring storage variables that are not used.
They cause an increase in computations (and unnecessary gas consumption) and can decrease readability of the code
Non-Critical
Instances include:
GlobalAccessControl.sol:51: //storage variable transferFromDisabled never initialized or read.
Manual Analysis
Remove this storage variable
It is best to use as few local variables as possible to avoid unnecessary computations.
Non-Critical
Instances include:
StakedCitadel.sol: 773 uint256 _before = token.balanceOf(address(this)); //_pool is already initialized with token.balanceOf(address(this));
Manual Analysis
remove the declaration of _before line 773, and replace _before line 776 with _pool
For clarity, you can rename _before and _after as _poolBefore and _poolAfter
cachedXCitadel is an instance of IVault, not an address. Even if the code compiles and works, the syntax in the deposit() call is misleading.
Non-Critical
Instances include:
CitadelMinter.sol: 195 IVault(cachedXCitadel).deposit(lockingAmount); //cachedXCitadel is not an address
Manual Analysis
replace IVault(cachedXCitadel) with
cachedXCitadel
uint is an alias for uint256.
It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint
Non-Critical
Instances include:
Funding.sol: 224 uint citadelAmount = getAmountOut(_assetAmountIn); Funding.sol: 419 uint _citadelPriceInAsset;
Manual Analysis
replace uint with
uint256
🌟 Selected for report: Dravee
Also found by: 0v3rf10w, 0x1f8b, 0xAsm0d3us, 0xBug, 0xDjango, 0xNazgul, 0xkatana, CertoraInc, Cityscape, Funen, Hawkeye, IllIllI, MaratCerby, SolidityScan, TerrierLover, TomFrenchBlockchain, Tomio, TrungOre, bae11, berndartmueller, csanuragjain, defsec, delfin454000, ellahi, fatherOfBlocks, gs8nrv, gzeon, horsefacts, ilan, jah, joestakey, joshie, kebabsec, kenta, nahnah, oyc_109, rayn, rfa, robee, saian, securerodd, simon135, slywaters, sorrynotsorry, tchkvsky, teryanarmen, z3s
181.1552 USDC - $181.16
If 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.
Instances include:
scope: mintAndDistribute()
citadelToken is read twiceCitadelMinter.sol:178 CitadelMinter.sol:217
scope: claimAssetToTreasury()
asset is read 3 timesFunding.sol:339: Funding.sol:341: Funding.sol:343:
scope: updateCitadelPriceInAsset()
minCitadelPriceInAsset is read twiceFunding.sol:428 Funding.sol:434
maxCitadelPriceInAsset is read twiceFunding.sol:429 Funding.sol:435
scope: buy()
saleStart is read twiceKnightingRound.sol:167 KnightingRound.sol:169
totalTokenIn is read twiceKnightingRound.sol:174 KnightingRound.sol:198
guestlist is read twiceKnightingRound.sol:178 KnightingRound.sol:179
scope: depositFor()
token is read 3 timesStakedCitadel.sol:773 StakedCitadel.sol:774 StakedCitadel.sol:775
scope: _withdraw()
token is read 3 timesStakedCitadel.sol:815 StakedCitadel.sol:819 StakedCitadel.sol:831
scope: _depositForWithAuthorization()
guestlist is read twiceStakedCitadel.sol:793 StakedCitadel.sol:795
scope: setStrategy()
strategy is read twiceStakedCitadel.sol:505 StakedCitadel.sol:507
scope: earn()
strategy is read twiceStakedCitadel.sol:722 StakedCitadel.sol:723
scope: withdraw()
vesting is read twiceStakedCitadel.sol:830 StakedCitadel.sol:831
scope: getMintableDebug()
globalStartTimestamp is read (5 + endingEpoch - lastEpoch) times:
number of reads depending on lastMintTimestamp as it is in a for loopSupplySchedule.sol:180 SupplySchedule.sol:184 SupplySchedule.sol:197 SupplySchedule.sol:200 SupplySchedule.sol:204 SupplySchedule.sol:211 SupplySchedule.sol:212
Manual Analysis
cache these storage variables in memory
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.
Instances include:
scope: initialize()
CitadelToken.sol:22: string memory _name, string memory _symbol
scope: initializeNewRole()
GlobalAccessControl.sol:107: string memory roleString
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
Manual Analysis
Replace memory with calldata
>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
Instances include:
scope: _transferToFundingPools()
CitadelMinter.sol:343
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
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
scope: vest()
StakedCitadelVester.sol:138
scope: getEpochAtTimestamp()
SupplySchedule.sol:61
scope: getMintable()
SupplySchedule.sol:91
scope: getMintableDebug()
SupplySchedule.sol:180
Manual Analysis
Replace > 0 with !0
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
Instances include:
CitadelMinter.sol:272
Funding.sol:172 Funding.sol:178 Funding.sol:270 Funding.sol:271
KnightingRound.sol:120 KnightingRound.sol:167 KnightingRound.sol:173 KnightingRound.sol:259 KnightingRound.sol:275 KnightingRound.sol:293
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:111
SupplySchedule.sol:111 SupplySchedule.sol:142 SupplySchedule.sol:208
Manual Analysis
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 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
Instances include:
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: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:95 GlobalAccessControl.sol:100 GlobalAccessControl.sol:112 GlobalAccessControl.sol:116
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: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:64 StakedCitadelVester.sol:65 StakedCitadelVester.sol:137 StakedCitadelVester.sol:138
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
Manual Analysis
Replace require statements with custom errors.
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.
Instances include:
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:103: uint256 mintable = 0; SupplySchedule.sol:192: uint256 mintable = 0;
Manual Analysis
Remove explicit initialization for default values.
When emitting an event, using a local variable instead of a storage variable saves gas.
Instances include:
Funding.sol: 343
SupplySchedule.sol: 197
Manual Analysis
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
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
Instances include:
StakedCitadel.sol: 780
Manual Analysis
Inline this function where it is called:
StakedCitadel.sol: 310 StakedCitadel.sol: 323 StakedCitadel.sol: 330 StakedCitadel.sol: 342
Prefix increments are cheaper than postfix increments.
Instances include:
CitadelMinter.sol:152 CitadelMinter.sol:344
SupplySchedule.sol:208
Manual Analysis
change i++ to ++i
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.
Instances include:
CitadelMinter.sol:266
Manual Analysis
poolExists line 261 and inline the computation of fundingPools.contains(_pool) line 266 and 273.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
Instances include:
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;
Manual Analysis
Place citadelPriceFlag after SaleRecipient to save one storage slot
uint256 public assetDecimalsNormalizationValue; address public citadelPriceInAssetOracle; address public saleRecipient; +bool public citadelPriceFlag;
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.
Instances include
KnightingRound.sol:69 KnightingRound.sol:70 KnightingRound.sol:76 KnightingRound.sol:162
Manual Analysis
Replace uint8 with uint256
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
Instances include:
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: 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: 198: totalTokenIn + _tokenInAmount is capped by tokenInLimit, overflow check unnecessary KnightingRound.sol: 250: totalTokenIn is less than tokenInLimit, underflow check unnecessary
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: 208: i is capped by endingEpoch, overflow check unnecessary
Manual Analysis
Place the arithmetic operations in an unchecked block