search

code-423n4 / 2022-04-badger-citadel-findings

0 stars 1 forks source link

QA Report #223

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

Check zero denominator

PROBLEM

When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol:225:    xCitadel.getPricePerFullShare()

StakeCitadel.sol

StakeCitadel.sol:808:   totalSupply()
StakeCitadel.sol:890:   _pool

TOOLS USED

Manual Analysis

MITIGATION

Before doing these computations, add a non-zero check to the variables aforementioned.

nonReentrant modifier unused

PROBLEM

Some external functions calling the ERC20 methods safeTransfer or safeTransferFrom do not have the nonReentrant modifier and are hence unprotected to reentrancy (besides the gas limit on the methods)

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

CitadelToken.sol

Funding.sol: 334 claimAssetToTreasury()

KnightingRound.sol

KnightingRound.sol: 209 claim()

StakedCitadel.sol

StakedCitadel.sol: 698 sweepExtraToken(address _token)
StakedCitadel.sol: 717 earn()

StakedCitadelVester.sol

StakedCitadelVester.sol: 85 claim(address recipient, uint256 amount)

TOOLS USED

Manual Analysis

MITIGATION

Use the nonReentrant modifier on these functions.

Unbounded for loop

PROBLEM

There is no restriction on the numbers of pool added, either in _addFundingPool() or setFundingPoolWeight(). Hence, the for loop in CitadelMinter.sol is unbounded, which could result in the gas required to perform the function call exceeding the gas limit if the number of pools is too high, preventing the distribution of citadel tokens to the funding pools.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol:344:

TOOLS USED

Manual Analysis

MITIGATION

Add an upper limit to the total amount of funding pools, and add a check before adding a pool in setFundingPoolWeight(): 274

Typos

PROBLEM

There are a few typos in some comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol: 101 expection

Funding.sol

Funding.sol: 289 cumulatiive

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol:110 address _citadelPriceInAssetOracle

GlobalAccessControl.sol

GlobalAccessControl.sol: 107 bytes32 role, string memory roleString,bytes32 adminRole

KnightingRound.sol

KnightingRound.sol: 109 address _globalAccessControl
KnightingRound.sol: 209 uint256 tokenOutAmount_

StakedCitadel.sol

StakedCitadel.sol: 175 address _vesting
StakedCitadel.sol: 363 bytes32[] memory proof

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol: 143 getFundingPoolWeights()
CitadelMinter.sol: 314 initializeLastMintTimestamp()
CitadelMinter.sol: 338 _transferToFundingPools(uint256 _citadelAmount)
CitadelMinter.sol: 362 _removeFundingPool(address _pool)
CitadelMinter.sol: 374 _addFundingPool(address _pool)

Funding.sol

Funding.sol: 278 clearCitadelPriceFlag()
Funding.sol: 383 setSaleRecipient(address _saleRecipient)
Funding.sol: 397 setCitadelAssetPriceBounds(uint256 _minPrice, uint256 _maxPrice)

GlobalAccessControl.sol

GlobalAccessControl.sol: 94 pause()
GlobalAccessControl.sol: 99 unpause()

KnightingRound.sol

KnightingRound.sol: 109 address _globalAccessControl
KnightingRound.sol: 209 uint256 tokenOutAmount_

StakedCitadelVester.sol

StakedCitadelVester.sol: 59 initialize(address _gac, address _vestingToken, address _vault)

SupplySchedule.sol

SupplySchedule.sol: 43 initialize(address _gac)
SupplySchedule.sol: 67 getCurrentEpoch()
SupplySchedule.sol: 71 getEmissionsForEpoch(uint256 _epoch)
SupplySchedule.sol: 79 getEmissionsForCurrentEpoch()
SupplySchedule.sol: 84 getMintable(uint256 lastMintTimestamp)
SupplySchedule.sol: 132 setMintingStart(uint256 _globalStartTimestamp)
SupplySchedule.sol: 150 setEpochRate(uint256 _epoch, uint256 _rate)

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

KnightingRound.sol

KnightingRound.sol: 47 mapping(address => uint256) public boughtAmounts;
KnightingRound.sol: 51 mapping(address => bool) public hasClaimed;

StakedCitadel.sol

StakedCitadel.sol: 94:     mapping(address => uint256) public additionalTokensEarned;
StakedCitadel.sol: 95:     mapping(address => uint256) public lastAdditionalTokenAmount;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, for the StakedCitadel.sol mappings, the mitigation could be:

struct AdditionalTokens {
  uint256 tokensEarned;
  uint256 lastAmount;
}

And it would be used as a state variable:

mapping(address =>  AdditionalTokens) additionalTokens;

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol: 328 Sweep(_token, amount) //emitted before amount transferred to saleRecipient

KnightingRound.sol

KnightingRound.sol: 413 Sweep(_token, amount) //emitted before amount transferred to saleRecipient

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Unused storage variable

PROBLEM

Though they are allowed in Solidity and do not pose a direct security issue, it is best practice to avoid declaring storage variables that are not used.

They cause an increase in computations (and unnecessary gas consumption) and can decrease readability of the code

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

GlobalAccessControl.sol

GlobalAccessControl.sol:51:
    //storage variable transferFromDisabled never initialized or read.

TOOLS USED

Manual Analysis

MITIGATION

Remove this storage variable

Redundant variable declaration

PROBLEM

It is best to use as few local variables as possible to avoid unnecessary computations.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

StakedCitadel.sol

StakedCitadel.sol: 773

uint256 _before = token.balanceOf(address(this));

//_pool is already initialized with token.balanceOf(address(this));

TOOLS USED

Manual Analysis

MITIGATION

remove the declaration of _before line 773, and replace _before line 776 with _pool

For clarity, you can rename _before and _after as _poolBefore and _poolAfter

Incorrect variable type

PROBLEM

cachedXCitadel is an instance of IVault, not an address. Even if the code compiles and works, the syntax in the deposit() call is misleading.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CitadelMinter.sol

CitadelMinter.sol: 195

IVault(cachedXCitadel).deposit(lockingAmount);
//cachedXCitadel is not an address

TOOLS USED

Manual Analysis

MITIGATION

replace IVault(cachedXCitadel) with cachedXCitadel

Uint256 alias

PROBLEM

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Funding.sol

Funding.sol: 224
uint citadelAmount = getAmountOut(_assetAmountIn);

Funding.sol: 419
uint _citadelPriceInAsset;

TOOLS USED

Manual Analysis

MITIGATION

replace uint with uint256