Badger Citadel contest - shenwilly's results

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L291-L295

Vulnerability details

Impact

Calling balance only returns the balance of underlying token in StakedCitadel, while it should also includes tokens deposited in the strategy. As balance is heavily used within the contract, this wrong value will cause a lot of problems.

In particular, withdrawal uses balance to calculate the amount of token to withdraw per share. If a user tries to withdraw when tokens are deposited in the strategy, they will receive less amount than they should actually get.

Recommended Mitigation Steps

Include the token balance of strategy in balance().

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L202-L216 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L177

Vulnerability details

Impact

When funding.discount is set to 0, calling getAmountOut will always return 0 instead of the actual amount without discount. As a result, users calling deposit will receive 0 xCitadel, essentially losing their funds. This is particularly dangerous as the contract is initialized with discount value of 0.

Proof of Concept

citadelAmount_ is not assigned value when funding.discount is 0.

Recommended Mitigation Steps

Modify L207-2013 to:

citadelAmount_ = _assetAmountIn * citadelPriceInAsset;

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

or alternatively, add a new conditional statement for funding.discount == 0.

Low Risk Vulnerabilities

1. Missing zero address check on strategist

While StakedCitadel.initialize has a zero-address check on strategist, there is no such check in SettAccessControl.setStrategist it inherits. Strategist being set to zero address would cause StakedCitadel.reportAdditionalToken to revert.

Mitigation

Add zero address check in setStrategist.

2. Dangerous governance update

Passing a wrong address to SettAccessControl.setGovernance would mean losing access to governance functions, and would require a contract redeployment to fix.

Mitigation

Use two-step process (set pending governance and accepting the governance) for setting new governance address.

3. Unused variable and comment mismatch: transferFromDisabled

GlobalAccessControl.transferFromDisabled is not used anywhere. The comment stated that it should be set to true in initialize, but there is no such statement in the initialize function.

Mitigation

Remove if unused or implement it according to the specification.

4. Comment mismatch: deposit

Funding.deposit comment mistakenly defines _minCitadelOut param as "ID of DAO to vote for".

Mitigation

Change the comment to "the minimal amount of citadel token to receive".

5. Missing checks for min <= max

Some functions are missing require statement that checks whether min param is lower/equal than max param:

• setDiscountLimits, setting _minDiscount higher than _maxDiscount would make setDiscount to always revert.
• setCitadelAssetPriceBounds, setting _minPrice higher than _maxPrice would cause citadelPriceFlag to always be set to true whenever updateCitadelPriceInAsset is called.

Mitigation

Add the comparison checks.