QA Report

[code423n4]

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.