Badger Citadel contest - tchkvsky's results

Declare modifiers together

Handle

tchkvsky

Scope

Funding.sol

GlobalAccessControlManaged.sol

Vulnerability details

onlyCitadelPriceInAssetOracle() modifier in Funding.sol should be declared after the events.

Also, the modifiers declared in GlobalAccessControlManaged.sol should be placed before any function declaration, specifically, __GlobalAccessControlManaged_init(). This will aid contract readability.

Tools Used

Manual review

Recommendation

Declare modifiers together. Group events together. Follow the recommended contract layout. Inside each contract, library or interface, use the following order:

1. Type declarations

2. State variables

3. Events

4. Modifiers

5. Functions

Useful link:

Order Of Layout | Style Guide ！ Solidity 0.8.14 documentation

---

Missing or incomplete natspec comments

Scope

CitadelMinter.sol

Funding.sol

GlobalAccessControl.sol

KnigthingRound.sol

StakedCitadel.sol

StakedCitadelVester.sol

SupplySchedule.sol

Vulnerability details

Missing or incomplete natspec comments in contracts

Proof of Concept

These functions do not have a comment or partially commented:

CitadelMinter.getFundingPoolWeights()

CitadelMinter.initializeLastMintTimestamp()

CitadelMinter._transferToFundingPools()

CitadelMinter._removeFundingPool()

CitadelMinter._addFundingPool()

Funding.clearCitadelPriceFlag()

Funding.setSaleRecipient()

Funding.setCitadelAssetPriceBounds()

GlobalAccessControl.pause()

GlobalAccessControl.unpause()

KnightingRound.claim() - no @return tag

StakedCitadel.depositFor() - no @param tag for third parameter i.e proof

StakedCitadel._depositWithAuthorization() - no @param tags

StakedCitadel._depositForWithAuthorization() - no @param tags

StakedCitadelVester.initialize()

SupplySchedule.sol

Tools Used

Manual review

Recommendation

Include comments in contracts where there is none. Update contracts which are partially commented

---

Handle

tchkvsky

Scope

SettAccessControl.sol#L37-L54

Vulnerability details

Missing zero address validation in permissioned actors

Impact

Alice calls setGovernance without specifying the _governance, so Alice loses ownership of the contract.

Proof of Concept

SettAccessControl.setStrategist

SettAccessControl.setKeeper

SettAccessControl.setGovernance

Tools Used

Manual review

Recommendation

Check that the address is not zero.

---

Complex pragma versions used in interfaces

Handle

tchkvsky

Vulnerability details

Complex pragma versions is used in interfaces:

pragma solidity >=0.5.0<=0.9.0 is too complex

Impact

Different versions of solidity might have different bugs that might cause unwanted behavior in contracts. It is recommended to use a single pragma version.

Proof of Concept

(src/interfaces/badger/IBadgerGuestlist.sol#2) 
(src/interfaces/badger/IBadgerVipGuestlist.sol#2) 
(src/interfaces/badger/IStrategy.sol#3) 
(src/interfaces/badger/IVault.sol#3) 
(src/interfaces/citadel/ICitadelToken.sol#3) 
(src/interfaces/citadel/IGac.sol#3) 
(src/interfaces/citadel/IMedianOracle.sol#3) 
(src/interfaces/citadel/IStakedCitadelLocker.sol#3) 
(src/interfaces/citadel/ISupplySchedule.sol#3) 
(src/interfaces/citadel/IVesting.sol#2) 
(src/interfaces/convex/BoringMath.sol#2) 
(src/interfaces/convex/IRewardStaking.sol#2) 
(src/interfaces/convex/IStakingProxy.sol#2) 
(src/interfaces/convex/MathUtil.sol#2) 
(src/interfaces/erc20/IERC20.sol#3)

Tools Used

Static analyzer

Recommendation

Consider modifying contracts to use a single pragma version. Alternatively, use a modern interface with a stable, yet single solidity version.

Pack variables together to save gas

Handle

tchkvsky

Scope

Funding.sol

StakedCitadelLocker.sol

Vulnerability details

uint256, mapping, bytes32, string all take 32 bytes in a slot. uint8 uses 1 byte (StakedCitadelLocker.sol), bool uses 1 byte, whileaddress uses 20 bytes. Consider rearranging and packing values that can fit into a slot (i.e. 32 bytes) together. An example is packing a bool and address, since both will only use 21 bytes out of the 32 bytes in a slot (packing can be done in Funding.sol).

This also applies to the values in a struct. They can be packed together.

Tools Used

Manual review

Recommendation

Declare variables that occupy 32 bytes first, then declare the others according to their sizes, in decreasing order. Alternatively, pack variables with smaller size into others (that can accommodate) till a slot is used up.

---

Prefix increments/decrements are cheaper than the postfix form

Handle

tchkvsky

Vulnerability details

These functions do not use prefix increments/decrements (++i/--i)

Impact

Using prefix increment/decrement is more gas efficient

Proof of Concept

CitadelMinter.sol #L152

StakedCitadelLocker.sol #L243, #L322, #L373, #L412, #L464, #L493, #L516, #L535, #L567, #L806, #L953, #L1035

SupplySchedule.sol #L208

GlobalAccessControlManaged.sol#L48

Tools Used

Manual review

Recommendation

Consider using prefix increments/decrements (++i / --i) to save gas

---

public function should be made external

Handle

tchkvsky

Scope

SettAccessControl.sol#L51-54

Vulnerability details

setGovernance() is a public function and it is never called from the contract. It should be declared external instead. This will save some gas.

Tools Used

Manual review

Recommendation

Use external specifier for functions that are never called from the contract.

---

Variable reinitialized to its default value

Handle

tchkvsky

Scope

GlobalAccessControlManged.sol#L47

Vulnerability details

Variable reinitialized to its default value.

Impact

The default value for a bool is false and the validRoleFound variable is reinitialized to false. This wastes gas.

Proof of Concept

bool validRoleFound = false;

Tools Used

Manual review

Recommendation

Remove explicit initialization for default values

---

Use lower values of typeuint instead of uint256 to store hardcoded values

Handle

tchkvsky

Vulnerability details

The state variable MAX_BPS is declared using uint256 which takes 32 bytes. uint14 can be used to store this as the maximum value (i.e. 10,000) is still within range. Also, fundingBps, stakingBps, lockingBps declared in state but used locally should be checked. Also, the parameter _weight in CitadelMinter.setFundingPoolWeight() is between 0 and 10,000 (inclusive). uint14 should be able to store this value for _weight. uint256 uses 32 bytes for storing the value while uint14 almost uses 2 bytes. This will save gas.

Other contracts to consider:

StakedCitadelLocker.sol, StakedCitadelVester.sol

### Proof of Concept

CitadelMinter.sol #L42, #L244-L284,

Funding.sol #L30

StakedCitadel.sol #L68-L117

• ONE_ETH can be stored in uint64

• MAX_BPS can be stored in uint14

• SECS_PER_YEAR can be stored in uint26

• WITHDRAWAL_FEE_HARD_CAP and MANAGEMENT_FEE_HARD_CAPcan be stored in uint8

• PERFORMANCE_FEE_HARD_CAP can be stored in uint12

Tools Used

Manual review

Recommendation

Use lower values of type uint instead of uint256 when storing hardcoded integers.