Badger Citadel contest - 0xkatana's results

1. Low - safeApprove is deprecated

Impact

The _setupRole function is deprecated according to the Open Zeppelin comment Deprecated. This function has issues similar to the ones found in {IERC20-approve}, and its usage is discouraged.

Proof of concept

safeApprove instances in code https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L142 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L133-L136

This Open Zeppelin comment indicates it is deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L39-L43

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace safeApprove with {safeIncreaseAllowance} or {safeDecreaseAllowance} instead

2. Low - Deprecated _setupRole function used

Impact

The _setupRole function is deprecated according to the Open Zeppelin comment NOTE: This function is deprecated in favor of {_grantRole}

Use the recommended _grantRole function instead.

Proof of concept

The _setupRole function, which is deprecated, is found in two places https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/GlobalAccessControl.sol#L69-L71

This Open Zeppelin comment indicates it is deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L195

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

3. Low - Incorrect comment in sweep

Impact

The sweep function has this comment

/**
 * @notice Transfers out any tokens accidentally sent to the contract. Can only be called by owner
 * @dev The contract transfers all `asset` directly to `saleRecipient` during a sale so it's safe
 *      to sweep `asset`. For `citadel`, the function only sweeps the extra amount
 *      (current contract balance - amount left to be claimed)
 * @param _token The token to sweep
 */

But there is no code to sweep asset in the amount of "contract balance - amount" as described. This comment does not match the code and needs modifying.

Proof of concept

The incorrect comment https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L308-L314

Tools Used

Manual analysis

Recommended Mitigation Steps

The comment should say "For citadel, use claimAssetToTreasury" and remove the claim about specific amounts.

1. Use prefix not postfix in loops

Impact

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Proof of Concept

There are three examples of this https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L208 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L48

Tools Used

Manual analysis

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

2. Use calldata instead of memory for function parameters

Impact

Use calldata instead of memory for function parameters. Having function arguments use calldata instead of memory can save gas.

Proof of Concept

There are many cases of function arguments using memory instead of calldata

Tools Used

Manual analysis

Recommended Mitigation Steps

Change function arguments from memory to calldata

3. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L301
2. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L321
3. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L328
4. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L370
5. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L377
6. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L148
7. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L298
8. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L325
9. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L390
10. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L96
11. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L143
12. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L157

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

4. Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

There are several places where an int is initialized to zero, which looks like

uint256 amount = 0;

Instances in code: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L103 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L192 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L180-L182

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 amount;

5. Cache array length before loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Proof of Concept

This optimization is already used in some places, but is not used in the GlobalAccessControlManaged lib https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L48

Tools Used

Manual analysis

Recommended Mitigation Steps

Cache the array length before the for loop

6. Replace _setupRole call with _grantRole

Impact

The _setupRole function calls _grantRole, so it would save gas to call _grantRole directly

Proof of concept

The _setupRole function, which is deprecated, is found in two places https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/GlobalAccessControl.sol#L69-L71

This Open Zeppelin _setupRole code shows it is deprecated and only calls _grantRole https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L197-L199

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

7. Use != 0 instead of > 0

Impact

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Proof of Concept

Locations where this was found include https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L270 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L343 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L170 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L209 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L322 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L340 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L424 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L452 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L835 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L908 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L125 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L129 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L172 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L184 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L215 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L313 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L332 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L411 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L138

Tools Used

grep

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

8. Add unchecked to getAmountOut in Funding

Impact

Use unchecked when there is no overflow risk to save gas

Proof of Concept

This is lines 209-213 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L209-L213

if (funding.discount > 0) {
    citadelAmount_ =
        (citadelAmountWithoutDiscount * MAX_BPS) /
        (MAX_BPS - funding.discount);
}

The entire citadelAmount_ calculation line can be unchecked because

1. funding.discount <= funding.maxDiscount from the setDiscount function and funding.maxDiscount < MAX_BPS from the setDiscountLimits function, so funding.discount < MAX_BPS meaning the subtraction will not overflow
2. There will never be divide by zero because MAX_BPS and funding.discount will never be equal using the logic from point 1

Tools Used

Manual analysis

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings

9. Add unchecked in SupplySchedule

Impact

Use unchecked when there is no overflow risk to save gas

Proof of Concept

Several lines in SupplySchedule are not at risk of overflow, underflow, or divide by zero cases and can use unchecked to save gas https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L108

This can be unchecked because block.timestamp >= cachedGlobalStartTimestamp and epochLength is a non-zero constant so the value cannot create division problems.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings

10. Add unchecked in getTokenInLimitLeft

Impact

Use unchecked when there is no overflow risk to save gas

Proof of Concept

The getTokenInLimitLeft function is not at risk of underflow and can use unchecked to save gas https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L249-L251

This can be unchecked because the check totalTokenIn < tokenInLimit will prevent underflow in tokenInLimit - totalTokenIn

Tools Used

Manual analysis

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings

11. Unnecessary reentrant modifier

Impact

The sweep function follows the checks-effects-interactions pattern for preventing reentrancy. The reentrant modifier is unnecessary and wastes gas.

Proof of Concept

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

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Add nonreentrant modifier only when reentrancy is a risk. Remove the reentrancy import in the KnightingRound contract