Badger Citadel contest - sorrynotsorry's results

QA Report (Low & Non-Critical)

• There is no zero value check at the denominators to prevent DoS and for the explicity: CitadelMinter.sol#L348, Funding.sol#L212, Funding.sol#L225, KnightingRound.sol#L241, StakedCitadel.sol#L890,

• At SupplySchedule.sol#L4 and SupplySchedule.sol#L21 redundant import and DSTest can be removed.

• TODO's can be removed from Funding.sol#L15, Funding.sol#L61, Funding.sol#L183, GlobalAccessControl.sol#L106 ,KnightingRound.sol#L14 , SupplySchedule.sol#L159

• Usage of deprecated safeApprove. Link

• Floating Pragma used in CitadelToken.sol, GlobalAccessControl.sol, GlobalAccessControlManaged.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) 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. Reference

• Since safeTransfer method uses transfer with 2300 gas stipend which is not adjustable,it may likely to get broken in future in case of hardforks causing gas price changes or when calling a contract's fallback function. Reference Link -1, Reference Link -2

• Contracts' Solidity compiler versions vary from 0.5.0 to 0.8.12. It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks also for maintenance purposes.

• Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Instead abi.encode() can be used. Reference Link

• The require statements in initialize() method of StakedCitadel.sol don't throw error. In case of any error pops up whilst initiliazation, it will not be possible to know the error source. Also pause() method of GlobalAccessControlManaged.sol.

• There is no zero value or the existing value check against the minCitadelPriceInAsset and maxCitadelPriceInAsset in setCitadelAssetPriceBounds() method. The new upper and lower bounds should be checked in order to prevent leaving the existing price out of bonds which would freeze trading untill the flags are cleared.

• No zero address check implemented in CitadelToken.sol initialize() method for _gac

• Uninitialized state/local variables: Uninitialized state/local variables are assigned zero values by the compiler and may cause unintended results e.g. transferring tokens to zero address or a DoS for Math malfunction (negative value for uint). Explicitly initialize all state/local variables. There are many of these on the codebase.

• At SupplySchedule.sol#L95 under getMintable() method, logic should be >= instead of > amended as below;

require(
           block.timestamp >= lastMintTimestamp,
           "SupplySchedule: already minted up to current block"
       );

Else the emit events for the token mint will not be correct.

• At CitadelMinter.sol , the setCitadelDistributionSplit() method should have zero value checks against fundingBps, stakingBps, lockingBps to prevent unintentional setting to zero. Else associated methods will not function as intended.

• It's a good practice to include nonReentrant modifier to mass token distribution events like in claim() methods of KnightingRound.sol, StakedCitadelVester.sol

• The initialize() method of StakedCitadelVester.sol doesn't have a address(0) check for the _gac

• At Funding.sol initialize() method, there is no adress(0) or Zero value check for _gac, _citadel, _asset, _xCitadel, _assetCap

Gas Optimizations

• Emitting a storage value for the followings: CitadelMinter.sol#L269, CitadelMinter.sol#L354, Funding.sol#L343, Funding.sol#L434-435, Funding.sol#L461-462, StakedCitadel.sol#L424, StakedCitadelVester.sol#L148,StakedCitadelVester.sol#L150, SupplySchedule.sol#L196-198 The emitted values can all be cached and emitted accordingly to minimize usage of SLOADs

• At StakedCitadel.sol#L817 : The substraction can be unchecked since the condition is already met by if statement on StakedCitadel.sol#L816 and the substraction can not overflow.

• At Funding.sol#L236 : The substraction can be unchecked since the condition is already met by if statement on Funding.sol#L235 and the substraction can not overflow.

• At KnightingRound.sol#L250 : The substraction can be unchecked since the condition is already met by if statement on KnightingRound.sol#L249 and the substraction can not overflow.

• At StakedCitadelVester.sol#L112 : The substraction can be unchecked since the condition is already met by if statement on StakedCitadelVester.sol#L111 and the substraction can not overflow.

• At CitadelMinter.sol#L199 : The substraction can be unchecked since the condition is already met since lockingAmount is deposited to the vaul on line 195.

• At SupplySchedule.sol#L105 : The substraction can be unchecked since the condition is already met on SupplySchedule.sol#L99-100

• At StakedCitadelVester.sol#L112 and StakedCitadelVester.sol#L117 : The substraction can be unchecked since the condition is already met in claimableBalance() method's if statement.

• At StakedCitadel.sol#L818 , the function is calling withdraw method which again calls _withdraw method in the same contract (not inheriting other contracts' functions). However, for IStrategy(strategy).withdraw(_toWithdraw); statement, the logic can be inlined to prevent repetetive calls inside the same contract.

• Use bytes32 instead of string to save gas whenever possible since tring is a dynamic data structure and therefore is more gas consuming then bytes32. You could use bytes32 instead of string in the following places: StakedCitadel.sol#L85-L86 , inside StakedCitadel.sol initialize() method.

• Numerous constant' variables on many locations throughout the contracts can be declared as immutable to save gas.

• Choosing either named return or explicit instead of specifying both may reduce gas due to unnecessary bytecode introduced. There is an inconsistent use of implicit named return variables across the entire codebase which makes readability and maintainability hard. Below methods are only a few examples for this: At Funding.sol :

function deposit(uint256 _assetAmountIn, uint256 _minCitadelOut)
        external
        onlyWhenPriceNotFlagged
        gacPausable
        nonReentrant
        returns (uint256 citadelAmount_)

function getAmountOut(uint256 _assetAmountIn)
        public
        view
        returns (uint256 citadelAmount_)

function getStakedCitadelAmountOut(uint256 _assetAmountIn) public view returns (uint256 xCitadelAmount_)
function getRemainingFundable() external view returns (uint256 limitLeft_)
function getFundingParams() external view returns (FundingParams memory)
function getDiscount() external view returns (uint256)

• Using custom errors instead of returning strings saves gas. But due to the complexity and the unique methods of the project, this may not be the best approach. Then, trying to reduce the error strings less than 32 bytes is also a cheaper gas saving method.