search

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

0 stars 1 forks source link

QA Report #157

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Report

Check 0-address consistently or not at all

There are several initialize() function where a couple inputs are checked for the 0-address and others are not. The usefulness is up to debate but if you decide to do it, you should do it consistently. For example, the address of the GlobalAccessControl contract is checked in some functions but not in others.

KnightingRound doesn't initialize ReentrancyGuard

The KnightingRound contract's initialize() function doesn't initialize the inherited ReentrancyGuard.

Relevant code:

Add the following to the initialize() function:

__ReentrancyGuard_init();

Funding should verify that minPrice is always smaller than maxPrice

The Funding contract's minCitadelPriceInAsset and maxCitadelPriceInAsset values can be set through the setCitadelAssetPriceBounds() function. There, the contract doesn't verify that min < max. If, by accident, min > max, subsequent calls to updateCitadelPriceInAsset() will trigger the citadelPriceFlag which will pause all deposits until the issue is resolved.

Relevant code:

Add the following to the setCitadelAssetPriceBounds() function:

function setCitadelAssetPriceBounds(uint256 _minPrice, uint256 _maxPrice)
    external
    gacPausable
    onlyRole(CONTRACT_GOVERNANCE_ROLE)
{
    require(_minPrice < _maxPrice);

    // ...
}

SupplySchedule contract is not ready for production

The SupplySchedule contract still has some things left from testing. In the Funding contract there's also a test function which is properly commented as such. The SupplySchedule contract doesn't have that so I thought it's worth mentioning here.

inherits DSTest library

The SupplySchedule contract inherits a library used for testing purposes: ds-test

sets hardcoded epoch rates on initialization

In its initialize() function it calls the _setEpochRates() function which uses hardcoded values

multiple unused imports

The file imports multiple contracts/libraries that are not used.

open TODO to constraint the viable epochs

There's an open TODO to constraint the provided epoch in the setEpochRate() function to be in the future.

StakedCitadel keeps the treasury fees in the vault instead of the strategy

When a user withdraws funds from StakedCitadel, a portion of it is allocated to the treasury in form of fees. If the vault doesn't have enough tokens, it withdraws the necessary amount from the strategy. The user receives their share of the Citadel tokens while the treasury receives their share in form of xCitadel tokens. The underlying Citadel tokens of the treasury's xCitadel tokens are kept in the vault. They are not deposited back in to the strategy.

So this is not an actual vulnerability. Just from an economic standpoint it would make more sense to keep the treasuries Citadel tokens in the strategy so they earn yield. The best thing would be to withdraw only the user's Citadel tokens while leaving the treasuries amount in the strategy. Then mint xCitadel tokens corresponding to the fee to the treasury.

Relevant code:

KnightingRound's sale start, duration, and price shouldn't be modifiable after the sale started

Currently, the governance contract can modify the KnightingRound's saleStart. saleDuration, tokenOutPrice, and tokenInLimit state variables.

It's not a vulnerability. But, I believe it would provide for trust in the contract if these critical parameters are immutable after a sale has started. Otherwise, the governance is able to prolong a sale indefinitely. Or change the price midway through.

As far as I have understood it, this is a one time sale. In that case, I don't see the value in being able to update those values after it has started.

Relevant code:

A simple check for whether a sale has started would be:

require(saleStart == 0 || saleStart > block.timestamp, "sale has already started");

Contracts don't check for fee-on-transfer tokens consistently

For example, the StakedCitadel contract does check for fee-on-transfer tokens. It uses the actual number of tokens it received for the subsequent computation: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L773-L776

But, the KnightingRound and Funding contracts don't do that:

The assets will be determined by the deployer so again, not a direct vulnerability. But, as a best-practice I'd suggest adding the same check to the Funding and KnightingRound contracts.