Platform: Code4rena
Start Date: 14/04/2022
Pot Size: $75,000 USDC
Total HM: 8
Participants: 72
Period: 7 days
Judge: Jack the Pug
Total Solo HM: 2
Id: 110
League: ETH
Rank: 15/72
Findings: 3
Award: $1,220.73
🌟 Selected for report: 2
🚀 Solo Findings: 0
896.081 USDC - $896.08
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L291-L295 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L772-L776 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881-L893
The StakedCitadel contract's balance() function is supposed to return the balance of the vault + the balance of the strategy. But, it only returns the balance of the vault. The balance is used to determine the number of shares that should be minted when depositing funds into the vault and the number of shares that should be burned when withdrawing funds from it.
Since most of the funds will be located in the strategy, the vault's balance will be very low. Some of the issues that arise from this:
You can't deposit to a vault that already minted shares but has no balance of the underlying token:
balance == 0 and totalSupply == 10)pool == balance())You get more shares than you should
balance == 0 and totalSupply == 10)1 instead of 0.5 * 10 / 1 == 50 shares: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L890Now, the vault received 15 tokens. 10 from Alice and 5 from Bob. But Alice only has 10 shares while Bob has 50. Thus, Bob can withdraw more tokens than he should be able to.
It simply breaks the whole accounting of the vault.
The comment says that it should be vault's + strategy's balance: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L291-L295
Here's another vault from the badger team where the function is implemented correctly: https://github.com/Badger-Finance/badger-vaults-1.5/blob/main/contracts/Vault.sol#L262
none
Add the strategy's balance to the return value of thebalance() function like here.
#0 - GalloDaSballo
2022-04-23T01:40:52Z
Agree balance must have been changed by mistake or perhaps earn should not transfer to a strategy either would work
184.248 USDC - $184.25
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L177 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L202 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L184 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L769
The Funding contract's deposit() function uses the getAmountOut() function to determine how many citadel tokens the user should receive for their deposit. But, if no discount is set, the function always returns 0. Now the deposit() function tries to deposit 0 tokens for the user through the StakedCitadel contract. But, that function requires the number of tokens to be != 0. The transaction reverts.
This means, that no deposits are possible. Unless there is a discount.
Funding.deposit() calls getAmountOut(): https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L177
Here's the getAmountOut() function:
function getAmountOut(uint256 _assetAmountIn) public view returns (uint256 citadelAmount_) { uint256 citadelAmountWithoutDiscount = _assetAmountIn * citadelPriceInAsset; if (funding.discount > 0) { citadelAmount_ = (citadelAmountWithoutDiscount * MAX_BPS) / (MAX_BPS - funding.discount); } // unless the above if block is executed, `citadelAmount_` is 0 when this line is executed. // 0 = 0 / x citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue; }
Call to StakedCitadel.depositFor(): https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L184
require statement that makes the whole transaction revert: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L769
none
Change the getAmountOut() function to:
function getAmountOut(uint256 _assetAmountIn) public view returns (uint256 citadelAmount_) { uint256 citadelAmount_ = _assetAmountIn * citadelPriceInAsset; if (funding.discount > 0) { citadelAmount_ = (citadelAmount_ * MAX_BPS) / (MAX_BPS - funding.discount); } citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue; }
#0 - GalloDaSballo
2022-04-23T00:57:35Z
Agree, also dup
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, Hawkeye, Jujic, MaratCerby, Picodes, Ruhum, SolidityScan, TerrierLover, TomFrenchBlockchain, TrungOre, VAD37, Yiko, berndartmueller, cmichel, csanuragjain, danb, defsec, delfin454000, dipp, ellahi, fatherOfBlocks, georgypetrov, gs8nrv, gzeon, horsefacts, hubble, hyh, ilan, jah, joestakey, kebabsec, kenta, kyliek, m9800, minhquanym, oyc_109, p_crypt0, peritoflores, rayn, reassor, remora, rfa, robee, scaraven, securerodd, shenwilly, sorrynotsorry, tchkvsky, teryanarmen, z3s
140.3983 USDC - $140.40
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.
The KnightingRound contract's initialize() function doesn't initialize the inherited ReentrancyGuard.
Relevant code:
Add the following to the initialize() function:
__ReentrancyGuard_init();
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); // ... }
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.
The SupplySchedule contract inherits a library used for testing purposes: ds-test
In its initialize() function it calls the _setEpochRates() function which uses hardcoded values
The file imports multiple contracts/libraries that are not used.
There's an open TODO to constraint the provided epoch in the setEpochRate() function to be in the future.
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:
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");
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.