Wrong reweight implementation (ManagedIndexReweightingLogic.sol)

Lines of code Vulnerability details Impact The list of assets will be incorrect after reweight Proof of Concept Base on the doc, updatedAssets is the list of asset for the index after reweight. But the implementation just iterates throw all of the updatedAsset and checks if that weight of asset 0...

basePrice and quotePrice values are insufficiently validated

Lines of code Vulnerability details Impact The oracle data feed basePrice and quotePrice in refreshedAssetPerBaseInUQ of ChainlinkPriceOracle.sol will be stale which results in wrong basePrice value and quotePrice value. Proof of Concept In refreshedAssetPerBaseInUQ function, it calls...

UniswapV2PriceOracle.sol currentCumulativePrices() will revert when priceCumulative addition overflow

Lines of code Vulnerability details uint price0Cumulative, uint price1Cumulative, uint32 blockTimestamp = addresspair.currentCumulativePrices; Because the Solidity version used by the current implementation of UniswapV2OracleLibrary.sol is =0.8.7, and there are some breaking changes in Solidity...

Index Minting and Redemption Can Be Frontrun

Lines of code Vulnerability details Impact According to the provided source code, the user must transfer the underlying asset to the contract prior to calling mint or the index token before to calling burn. If these two actions are performed on the difference block, it introduces the risk that...

Attacker can cause minter to lose his funds by calling sync() between deposit and mint()

Lines of code Vulnerability details Impact All funds will be lost Proof of Concept To mint new index tokens underlying tokens need to be deposited in Vtoken.sol and then call the mint function. The mint function does balance-previousBalance to calculate tokens that were deposited. The previous...

UniswapV2PriceOracle#refreshedAssetPerBaseInUQ() will revert when pair cumulative prices underflow

Lines of code Vulnerability details function refreshedAssetPerBaseInUQaddress asset external override returns uint uint price0Cumulative, uint price1Cumulative, uint32 blockTimestamp = addresspair.currentCumulativePrices; uint32 timeElapsed = blockTimestamp - blockTimestampLast; if timeElapsed =...

ORACLE Data is not properly validated in ChainlinkPriceOracle.sol

Lines of code Vulnerability details Impact Price can be stale which can lead to wrong assetPerBaseInUQ return value Proof of Concept Oracle data feed is insufficiently validated. There is no check for stale price and round completeness. Tools Used Manual review, similar issue was found in yield...

Chainlink oracle data can be stale

Lines of code Vulnerability details Impact Oracle data can be stale which can lead to wrong calculations for balancing indexes. Proof of Concept When lastRoundData is called only price is pulled from the provided data. RoundId should be checked to ensure the data is updated. Recommended Mitigatio...

Chainlink's latestRoundData might return stale or incorrect results

Lines of code Vulnerability details Impact In ChainlinkPriceOracle.sol, latestRoundData is used but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation: Proof of Concept ChainlinkPriceOracle.solL83...

Index managers can rug user funds

Lines of code Vulnerability details Impact The ORDERERROLE role has the ability to arbitrarily transfer user funds, and this role is shared between both the orderer and people who can rebalance the index. Even if the owner is benevolent the fact that there is a rug vector available may negatively...

Wrong requirement in reweight function (ManagedIndexReweightingLogic.sol)

Lines of code Vulnerability details Impact The list of assets won't be changed after reweight because of reverted tx Proof of Concept requireupdatedAssets.length = IIndexRegistryregistry.maxComponents when reweight is not true, because as in the doc, maxComponent is the maximum assets for an inde...

Missing Validations for the return values of Chainlink Price feeds

Lines of code Vulnerability details Impact You check only the answerThe price after calling the chainlink Chainlink Price feeds in the following lines. In addition, you need to check whether the data is really updated. Proof of Concept Tools Used code review Recommended Mitigation Steps Please ad...

Index mint and burn calls can be front run

Lines of code Vulnerability details Impact Both in the mint and burn cases all the user supplied / due to a user assets can be stolen by an attacker, who detects correspondingly asset transfer calls / Index token transfer call and front runs Index contract's mint / burn call with own address as a...

Orderers Can Unauthorized Transfer User's Share In Single Step

Lines of code Vulnerability details Impact The transferFrom function of vToken.sol can be done without any user permissions or strict security checks, requires only the caller must has ORDERERROLE as the access control, exposing it to the centralize risk if an orderer is compromised or act...

New vest reset unlockBegin of existing vest without removing vested amount

Lines of code Vulnerability details Impact When vest is called by xCTDL vault, the previous amount will re-lock according to the new vesting timeline. While this is as described in L127, claimableBalance might revert due to underflow if vestingrecipient.claimedAmounts 0 because the user will need...

Stale price used when citadelPriceFlag is cleared

Lines of code Vulnerability details During the video it was explained that the policy operations team was meant to be a nimble group that could change protocol values considered to be safe. Further, it was explained that since pricing comes from an oracle, and there would have to be unusual...

first depositor in StakedCitadel can drain other depositors

Lines of code Vulnerability details in deposit, when the ratio totalSupply / balance is very high, the amount of the minted shares can round down to zero. Proof of Concept Let's say that the token is USDC. Alice is the first one to deposit in StakedCitadel. she deposits 1 basic unit of USDC 10-6...

Is Possible Attacker Block setStrategy() When Already Existing Strategy

Lines of code Vulnerability details Impact /// NOTE: Migrate funds if settings strategy when already existing one if strategy != address0 require IStrategystrategy.balanceOf == 0, "Please withdrawToVault before changing strat" ; When setStrategy was called, it require no fund in existing Strategy...

Funding: citadelPriceInAsset is only lazily updated

Lines of code Vulnerability details Impact When using citadelPriceInAsset in the getAmountOut function, since citadelPriceInAsset is updated lazily, this will make users use stale citadelPriceInAsset , causing users to suffer asset loss. Proof of Concept Tools Used None Recommended Mitigation Ste...

Accounts not on GuestList can deposit any amount in StakedCitadel

Lines of code Vulnerability details Impact All deposit functions trigger depositForWithAuthorization, which allows anyone to call depositFor regardless if the recipient is on guestList. This will make function setGuestList meaningless. Proof of Concept Provide direct links to all referenced code ...

Funding.getAmountOut returns zero when there is no discount set

Lines of code Vulnerability details Impact User provided asset funds will be lost, i.e. 100% to be frozen in the contract, as the system will not give away any Citadel in return. The issue is that when Funding's funding.discount is zero the getAmountOut will return zero for any given assetAmountI...

One could get up to 20x more xCTDL tokens when deposit right after earn().

Lines of code Vulnerability details Impact When earn is called by authorized actors keeper or governance, 95% of the balance of CTDL token in the StakedCitadel contract will be transferred to strategy. Thus, the balance will be roughly only 5% of the totalSupply. At this juncture, if an attacker...

GlobalAccessControl roles admin not set and CONTRACT_GOVERNANCE_ROLE can change admin even if it is not an admin

Lines of code Vulnerability details Impact Some roles doesn't have any admin set: TECHOPERATIONSROLE and TREASURYOPERATIONSROLE 1. Function 2 can change admin of existing roles, as there is no check whether the role exist already. Direct use case would be to change the admin role of...

Code credits fee-on-transfer tokens for amount stated, not amount transferred

Lines of code Vulnerability details Some ERC20 tokens, such as Tether USDT, allow for charging a fee any time transfer or transferFrom is called. If a contract does not allow for amounts to change after transfers, asset calculations will be incorrect. While the planned tokens to be used may not...

setStrategy() Can Happen Without Timelock

Lines of code Vulnerability details Impact /// @dev This is a rug vector, pay extremely close attention to the next strategy being set. /// Changing the strategy should happen only via timelock. function setStrategyaddress strategy external whenNotPaused onlyGovernance; requirestrategy != address...

centralization risk

Lines of code Vulnerability details Impact Can lead to unlimited minting of tokens Proof of Concept If any of the provided roles / actors get malicious, then unlimited number for tokens either for mint or redeem, can lead to loss for the protocol. It should be onlyadmin based or either should be...

admin can rug

Lines of code Vulnerability details the mint function in CitadelToken requires the role CITADELMINTERROLE. this role is managed by the governance: setRoleAdminCITADELMINTERROLE, CONTRACTGOVERNANCEROLE; therefore the admin can mint to himself an unlimited amount. --- The text was updated...

In Funding.sol#getAmountOut(), citadelAmount_ doesnot account for case of discount

Lines of code Vulnerability details Impact Function getAmountOutassetAmountIn fails to handle the case for funding.discount == 0 Proof of Concept function getAmountOutuint256 assetAmountIn public view returns uint256 citadelAmount uint256 citadelAmountWithoutDiscount = assetAmountIn...

deposit()ing when there is no discount results in zero xCitadel bought

Lines of code Vulnerability details The amount of citadel bought when there is no discount is always zero. If the user doesn't specify, or specifies zero as the minCitadelOut, then the user will get no xCitadel and will still have to pay the full price. Proof of Concept If funding.discount is equ...

If the funding discount is set to 0 in Funding.sol, getAmountOut() will return always 0, no matter the amount of asset in.

Lines of code Vulnerability details Impact If a user doesn't set a minimum set to 0 of citadel expected in function deposit, can receive 0 tokens in exchange, no matter the amount of asset he's depositing. Proof of Concept The funding contract is used to trade citadel token with another asset. A...

Chainlink's latestRoundData missing security checks

Lines of code Vulnerability details Impact Protocol uses Chainlink as one of the oracles that provides prices for the assets. Chainlink's latestRoundData is used but the implementation is missing important security checks that can result in stale and incorrect prices being returned. Proof of...

StakedCitadelVester/claimableBalance() has problems when users keep vesting.

Lines of code Vulnerability details Impact When users vest in StakedCitadelVester, the tokens will be locked in vest duration. Users can call claim to get back their tokens. The claimable amount is calculated in claimableBalance. Before the duration ends, the claimable amount is locked...

StakedCitadel depositors can be attacked by the first depositor with depressing of vault token denomination

Lines of code Vulnerability details Impact An attacker can become the first depositor for a recently created StakedCitadel contract, providing a tiny amount of Citadel tokens by calling deposit1 raw values here, 1 is 1 wei, 1e18 is 1 Citadel as it has 18 decimals. Then the attacker can directly...

KnightingRound: tokenOutPrice is only lazily updated

Lines of code Vulnerability details Impact In the KnightingRound contract, the tokenOutPrice is updated by the owner calling the setTokenOutPrice function, but this update is lazy and cannot guarantee that the user uses the latest tokenOutPrice. When setTokenOutPrice cannot update tokenOutPrice i...

loss of funds if there is no discount

Lines of code Vulnerability details citadelAmount is initialized to 0 by default, and then if the discount is greater than zero, it is set to the amount out. But if the discount is 0, it is not set to the amount out and citadelAmount stays 0 and then divided by assetDecimalsNormalizationValue, bu...

Linear vesting logic is incorrect for subsequent withdrawals

Lines of code Vulnerability details Impact After attempting a withdrawal, StakedCitadelVester.sol vest is called, creating a 21 day vesting schedule for the user to claim their withdrawed amount. This logic works perfectly for the first withdrawal, but will be incorrect for every subsequent...

Removing Provider could lead to no provider at all

Lines of code Vulnerability details Impact When removing a provider you don't verify that there is at least one provider left. If no provider is left, then no price data can be reported. Proof of Concept 1 Tools Used hardhat test Recommended Mitigation Steps Add a checks on the length of the...

TokenInLimit can be set higher than what the contract has to sell

Lines of code Vulnerability details Impact If totalTokenOutBought tokenOut.balanceOfaddressthis all tokens will be unclaimable and all funds would be lost. Proof of Concept TokenInLimit can be set arbitrarily high even if the contract doesn't have enough tokens to sell. Then users can buy more...

the governance can mint citadel tokens for themselves

Lines of code Vulnerability details the governance can call mint in citadel token and mint for themselves as much as they want and sell, which will cause the token price to drop to zero. --- The text was updated successfully, but these errors were encountered: All reactions...

IndexLogic: An attacker can burn other users' tokens to transfer assets to himself

Lines of code Vulnerability details Impact In the burn function of the IndexLogic contract, the user needs to transfer the tokens to the contract first, and then call the burn function to withdraw the assets. The attacker can monitor the number of tokens in the contract. When it is not 0, the...

Economic calculation may not be precise enough

Lines of code Vulnerability details In Funding.sol, the assetDecimalsNormalizationValue is set to 10 asset.decimals, later it is used to determine home many token units there are per one asset token in human representation. Firstly, this may be highly dangerous as .decimals return value isn't sai...

DoS at CitadelMinter.sol

Lines of code Vulnerability details Impact At CitadelMinter.sol, Funding Pool Weight can't be set at the beginning since totalFundingPoolWeight value is not assigned and Zero meanwhile being cached to newTotalWeight. Hence the substraction will not perform as it will yield to a negative value whi...

getPricePerFullShare() can be much lower than expected

Lines of code Vulnerability details Impact Since balance does not include the amount 95% that's been transferred to the strategy, the getPricePerFullShare may only give 5% of the intended price. getPricePerFullShare is used in other contracts, such as Funding.sol and may also be used in other...

The name of the function to setup a vesting in the interface IVesting.sol doesn’t match with the name of the function to setup a vesting in StakeCitadelVester.sol.

Lines of code Vulnerability details Impact Users will not be able to withdraw their funds . Proof of Concept When a user wants to withdraw his tokens from StakedCitadel.sol, vesting is supposed to be set and tokens are sent to the vesting contract where they are vested linearly for 21 days. This ...

Duplicate asset can be added

Lines of code Vulnerability details Impact Initialize function can be called multiple times with same asset. Calling with same asset will make duplicate entries in assets list. Any function reading assets will get impacted and would retrieve duplicate asset Proof of Concept 1. Observe that...

earn can be called many times

Lines of code Vulnerability details earn can be called many times and each time more money will be transferred to the strategy, and the money in the strategy can be above the desired percentage of the money that should be in the strategy. --- The text was updated successfully, but these errors we...

Asset Manager can update existing _assetAggregator

Lines of code Vulnerability details Impact Asset Manager can update the aggregator of an existing asset thus impacting all function making use of this asset. Ideally if an aggregator is already set for an asset the function should fail Proof of Concept 1. Asset Manager call function addAsset to...

Reliance on ERC20Upgradable.decimals() will always return 18 despite actual token decimals

Lines of code Vulnerability details Impact A call to ERC20UpgradeabletokenIn.decimals is used in the getAmountOut function of KnightingRound.sol to determine how much citadel to provide to the user for the given amount of tokenIn. The issue with using ERC20Upgradeable.decimals is that it always...

IVesting(vesting).setupVesting is not implemented in StakedCitadelVester.sol.

Lines of code Vulnerability details Impact When doing withdraw or withdrawAll in StakedCitadel.sol, it will call the internal function withdraw. The function then transfers tokens to the vesting contract, which should be StakedCitadelVester.sol. However, IVestingvesting.setupVesting is not...

DoS in StakedCitadel._withdraw() due to missing StakedCitadelVester.setupVesting() function

Lines of code Vulnerability details Contract StakedCitadelVester inherits from interface IVesting in fact it does not as it is missing the necessary is IVesting statement, but it's assumed to inherit from IVesting but wrongly implements the interface. The contract is expected to implement the...