Badger Citadel contest - kenta's results

2022-04-badger-citadel

1 delete unused import statement. The following import statement is never used.

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

Delete it.

2 delete an unused import statement. The following import statement is never used.

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

3 delete the unused library. The following using for is never used in the contract.

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

Delete them.

4 Lock pragmas to specific compiler version. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

In GlobalAccessControl.sol and CitadelToken.sol

pragma solidity 0.8.12;

5 wrong description. The following comment is not proper to explain the function finalize. If totalTokenIn is equal to or greater than tokenInLimit you can finalize the sale.

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

6 missing validation for input _tokenInLimit in setTokenInLimit. There is no validation for _tokenInLimit. I am not sure whether you intend or not. However if _tokenInLimit is less than totalTokenIn, the owner can immediately finalize the sale with the function finalize. If you do not intend it, try to use validation for _tokenInLimit to check whether the input _tokenInLimit is greater than totalTokenIn.

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

require(totalTokenIn < tokenInLimit);

2022-04-badger-citadel gas optimization

1 use cache for vesting[recipient].lockedAmounts + _amount and _unlockBegin + vestingDuration in vest. In Vest event vesting[recipient].lockedAmounts and vesting[recipient].unlockEnd will be also called, using cache must save gas costs.

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L140-L142 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L144 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L148 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L150

uint256 lockedAmounts = vesting[recipient].lockedAmounts + _amount; uint256 unlockedEnd = _unlockBegin + vestingDuration;

vesting[recipient].lockedAmounts = lockedAmounts; vesting[recipient].unlockBegin = _unlockBegin; vesting[recipient].unlockEnd = unlockedEnd;

emit Vest(recipient, lockedAmounts, unlockBegin, unlockedEnd);

2 make the calculation for _newTotalWeight simple in setFundingPoolWeight. You can make the following calculation simpler and save gas, especially deployment gas costs.

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

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

You can save gas according to the forge gas reporter like the following.

Deployment of CitadelMinter.sol median from 1876176 to 1874976 with this change setFundingPoolWeight() median from 40516 to 40507 with this change

3 delete unused cache. The cache for currentPoolWeight is used only one time in _removeFundingPool. You can delete it to save gas.

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

totalFundingPoolWeight = totalFundingPoolWeight - fundingPoolWeights[_pool];

4 use cache for saleStart in buy to save gas costs. saleStart will be used twice in this function.

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

uint256 _saleStart = saleStart; require(_saleStart <= block.timestamp, "KnightingRound: not started"); require(block.timestamp < _saleStart + saleDuration,"KnightingRound: already ended");

buy() median from 81357 to 81310 only with this change

5 use cache for totalTokenIn + _tokenInAmount in buy. totalTokenIn + _tokenInAmount will be used twice in buy. Use cache and save gas costs.

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

buy() median from 81357 to 81279 only with this change

6 delete cache amountLeftToBeClaimed in sweep. amountLeftToBeClaimed is used only one time in sweep.

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

if (_token == address(tokenOut)) { amount = amount - (totalTokenOutBought - totalTokenOutClaimed); }

7 use calldata instead of memory. In the following function, the input can be set as calldata to save gas.

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

string calldata roleString,

8 check the input _token earlier. The input _token must be checked earlier with the require statement, so you can save gas in case of reverting if _token is equal to address(asset).

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

Place the require statement for _token at the top of the function. 9 use !citadelPriceFlag instead of citadelPriceFlag == false.

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

require( !citadelPriceFlag, "Funding: citadel price from oracle flagged and pending review" );