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 =...

Potentially Incomplete or Stale Data from Oracle

Lines of code Vulnerability details Impact Calls to the Chainlink price oracle via refreshedAssetPerBaseInUQ in ChainlinkPriceOracle.sol use the correct function latestRoundData per Chainlink's documentation, but lacks the recommended validations to ensure that the round is complete and does not...

Unchecked parameters in Chainlink latestRounddata() could lead to retrieve stale data

Lines of code Vulnerability details Impact Oracle can retrieve unchecked stale data Proof of Concept In ChainlinkpriceOracle.sol the function function refreshedAssetPerBaseInUQaddress asset public override returns uint AssetInfo storage assetInfo = assetInfoOfasset; , int basePrice, , , =...

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...

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...

Tokens with fee on transfer are not supported

Lines of code Vulnerability details There are ERC20 tokens that charge fee for every transfer / transferFrom. Vault.soladdValue assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can ...

Should implement a periphery contract for user to mint indexToken

Lines of code Vulnerability details Impact User can lose their fund Proof of Concept When users want to mint an index token, users need to transfer their assets to addressvToken first, then call the mint function of IndexLogic.sol. If users make it into 2 transactions, miner can manipulate it/...

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...

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...

Inactive skipped assets can be drained from the index

Lines of code Vulnerability details Impact If an index has any inactive assets with the role SKIPPEDASSETROLE, a user can repeatedly deposit and withdraw assets, always getting the skipped asset without having to deposit any Proof of Concept During minting, any asset that has the 'skipped' role i...

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...

ERC20Gauges: The _incrementGaugeWeight function does not check the gauge parameter enough, so the user may lose rewards.

Lines of code Vulnerability details Impact The incrementGaugeWeight function is used to increase the user's weight on the gauge. However, in the incrementGaugeWeight function, it is only checked that the gauge parameter is not in deprecatedGauges, but not checked that the gauge parameter is in...

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...

Function name doesn't match the interface

Lines of code Vulnerability details Impact StakedCitadelVester contract doesn't support the IVesting interface, so the withdrawal of Citadel from the StakedCitadel contract will be impossible because the IVestingvesting.setupVestingmsg.sender, amount, block.timestamp; call will revert because the...

User funds stuck - buy works even after sale ends

Lines of code Vulnerability details Impact Buy function is not checking finalized value. Under specific scenario, Buy function will work even when sale has ended and finalize function has been called. If user buys any token at this moment then he wont be able to claim and will lose funds Proof of...

Initial StakedCitadel deposit with amount=1 wei causes very expensive share price leading to precision errors and loss of funds

Lines of code Vulnerability details Impact The first depositor into StakedCitadel is able to maliciously manipulate the share price by depositing the lowest possible amount 1 wei and then artificially blowing up the StakedCitadel Citadel token balance. Following depositors will loose their...

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...

Owner can takeover funds meant for distribution

Lines of code Vulnerability details Impact By calling sweep function at correct moment, Owner can transfer more than required tokenOut token which were meant to be distributed to users. Proof of Concept 1. Observe the sweep function function sweepaddress token external gacPausable nonReentrant...

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...

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...

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...

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...

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...

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...

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...

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 ...

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...

_mintSharesFor doesn’t check whether the pool is zero.

Lines of code Vulnerability details Impact In StakedCitadel.sol/mintSharesFor, it doesn’t check whether the pool is equal to zero. If pool == 0 and totalSupply != 0, mintSharesFor will revert. And In depositFor, it uses balance as pool. In consequence, when the CTDL balance of StakedCitadel is ze...

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...

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...

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...

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...

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...

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...

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...

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...

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...

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...

withdrawal amount might be wrong

Lines of code Vulnerability details r is the user's part of the contract balance, but is supposed to be the user's part of the total funds, including the strategy funds. therefore the check at line 816 will always return false because the user's part of the contract balance is smaller than the...

IndexLogic: An attacker can mint tokens for himself using assets deposited by other users

Lines of code Vulnerability details Impact In the mint function of the IndexLogic contract, users are required to transfer assets to vToken in advance, and then call the mint function to mint tokens. The attacker can monitor the asset balance in the vToken contract. When the balance is greater th...

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...

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...

Funding.deposit() doesn't work if there is no discount set

Lines of code Vulnerability details Impact 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...

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...

If there is no discount then buying citadel in funding will always return 0 citadel

Lines of code Vulnerability details Impact Users are unable to buy citadel in funding if the discount is 0. Proof of Concept In the funding contract if the discount is 0 then getAmountOut will always return 0 and users won't be able to use funding to buy citadel. If discount is 0 then the if is...

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...

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...

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...

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...