Badger Citadel contest - Funen's results

1. using pragma abicoder V2

Since CitadelMinter and GlobalAccessControl was using pragma experimental ABIEncoderV2 and used pragma 0.8.12, it because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2 instead since Solidity 0.7.4.

##POC

https://docs.soliditylang.org/en/develop/layout-of-source-files.html#spdx-license-identifier

##Tools

Manual Review

##Recommended Mitigation you can change it into pragma abicoder v2;

2. Unused comment

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L232

since no return was used here, it can be deleted or use it instead if necessary.

##Tool used Manual Review

3. Consistent Pragma

Since, CitadelMinter, KnightingRound, StakedCitadel, StakedCitadelVester, SupplySchedule -> 0.8.12 CitadelToken, GlobalAccessControl -> ^0.8.0

since CitadelToken & GlobalAccessControl can be using the same as the others for consistency pragma

##Tool Used Manual Review

4. Avoid multiple initializations

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L109

##POC Make sure though that you do not allow multiple initializations. For just a few parameters, simply add a check for each parameter, e.g., require(rate != 0). For many parameters, add an isInitialized boolean state variable:

such as e.g

bool isInitialized = false;

  function initialize(
    address _param1,
    address _param2,
    address _param3,
    address _param4,
  ) public {
    require(!isInitialized, 'Contract is already initialized!');
    isInitialized = true;

5. repair function order

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L361-L378

this function order can be change to minimizing error. so commonly function add() must be in order first then remove() after. it should be better on that way.

##Tool Used Manual Review

1. Saving gas by removing = 0

If a variable was not set/initialized, it is assumed to have default value to 0 this implementation was used for saving more gas by removing = 0

##POC https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6

##TOOLS USED Visual Studio Code, Manual Review

##Mitigation Step Remove = 0

##Occurances https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L180-L182

        uint256 lockingAmount = 0;
        uint256 stakingAmount = 0;
        uint256 fundingAmount = 0;

Another Occurance

main/src/CitadelMinter.sol#L366

2. Change uint256 i = 0 into uint i for saving more gas

this implementation can saving more gas for each loops.

##Tool Used Manual Review

##Recommended Mitigation Change it

Occurances

main/src/CitadelMinter.sol#L152
main/src/SupplySchedule.sol#L103
main/src/SupplySchedule.sol#L192

3. using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

main/src/CitadelMinter.sol#L152

4. Reorder Struct can saving gas

Since address was 20 bytes and uint256 was 32 bytes on struct, this implementation below can be used for gas opt

##Tool Used Manual Review

##Recommended Mitigation

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L46-L53

    struct FundingParams {
        address discountManager;
        uint256 discount;
        uint256 minDiscount;
        uint256 maxDiscount;
        uint256 assetCumulativeFunded; /// persistent sum of asset amount in over lifetime of contract.
        uint256 assetCap; /// Max asset token that can be taken in by the contract (defines the cap for citadel sold)
    }

5. Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

##Tool Used Manual Review

##Occurance :

main/src/CitadelMinter.sol#L301
main/src/CitadelMinter.sol#L321
main/src/CitadelMinter.sol#L328
main/src/CitadelMinter.sol#L370
main/src/CitadelMinter.sol#L377
main/src/Funding.sol#L148
main/src/Funding.sol#L298
main/src/Funding.sol#L325
main/src/StakedCitadelVester.sol#L137
main/src/StakedCitadelVester.sol#L138

6. Set value of constant to saving more gas

This implementation below can be saving more gas

##Tool Used Manual Review

##Recommended Mitigation

    uint256 public constant INITIAL_VESTING_DURATION = 86400 * 21; // 21 days of vesting

changed to

    uint256 public constant INITIAL_VESTING_DURATION = 1814400; // 21 days of vesting

7. vesting[msg.sender].claimedAmounts can be shorter

This implementation can be used for saving more gas, instead cache vesting[msg.sender].claimedAmounts it can be changed by using += instead.

##POC https://www.tutorialspoint.com/solidity/solidity_operators.htm

##Tool Used Manual Review

##Recommended Mitigation

            vesting[msg.sender].claimedAmounts += amount;