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

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

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

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

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

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

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

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

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

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

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

Funding contract fails without discount

Lines of code Vulnerability details Impact The deposit function of Funding.sol calls getAmountOut to determine the amount of citadel the user receives in exchange for an asset. The getAmountOut function contains a bug that results in the return being 0 if funding.discount is set to 0. In the...

Chainlink oracle might return stale data

Lines of code Vulnerability details Impact Oracle might return stale data for basePrice and quotePrice. Proof of Concept refreshedAssetPerBaseInUQ in ChainlinkPriceOracle.sol does not check if the data from Chainlink is fresh . If there is a problem with the Chainlink oracle, this contract may be...

latestRoundData might return stale

Lines of code Vulnerability details Impact The conctract uses latestRoundData but there is no check if the returned value is stale data or not Proof of Concept code-423n4/2021-12-perennial-findings24 code-423n4/2021-06-tracer-findings73 Tools Used Manual analysis Recommended Mitigation Steps...

Improper Validation Of Chainlink's latestRoundData Function

Lines of code Vulnerability details Impact When using Chainlink Price feeds, it is important to ensure the price feed data was updated recently. While getting started with chainlink requires just one line of code, it is best to add additional checks for "in production" environments. Here,...

Incorrect interface for StakedCitadelVester causes withdrawals to revert

Lines of code Vulnerability details The IVesting interface contains a setupVesting function: IVesting4 interface IVesting function setupVesting address recipient, uint256 amount, uint256 unlockBegin external; This function is called to send withdrawals to the vesting contract in...

missing a check

Lines of code Vulnerability details Impact in citadelMinter.setCitadelDistributionSplit there is a require check that check the sum of the propvalues must be 10000 bps and POLICYOPERATIONSROLE can set 2 of the variable to 0 and 1 to 10000 it is better to make a min value to the variables and chec...

Governor Infinitely Extend To Vesting Period

Lines of code Vulnerability details Impact During the review of StakedCitadelVester contract, It has been noticed governor role can extend vesting period indefinitely. There is no upper bound defined in the vesting period. Although, It is dependent on the governor, If governor private key is...

Should check return data from chainlink aggregators

Lines of code Vulnerability details Impact The refreshedAssetPerBaseInUQ function in the contract ChainlinkPriceOracle.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices. The...

StakedCitadel should implement EIP-4626

Lines of code Vulnerability details StakedCitadel should implement EIP-4626 StakedCitadel could be seen as an investment vault and therefore team should consider implementing the recent EIP-4626 standard, as it could enable a lot of potential integrations and would avoid having to deploy wrappers...

TECH_OPERATIONS_ROLE is not initialized properly

Lines of code Vulnerability details Impact setGuestlist and setTokenInLimit are unavailable for use during KnightingRound to use due to a mistake in initializing roles. Proof of Concept The role TECHOPERATIONSROLE, created in L30 of GlobalAccessControl.sol , is not initialized in the initialize t...

latestRoundData data insufficiently validated

Lines of code Vulnerability details Impact The data returned by the Chainlink latestRoundData function may be stale. There should be checks applied on the data received from Chainlink to validate that it is not stale. Proof of Concept The ChainlinkPriceOracle contract has these two lines , int...

Update initializer modifier to prevent reentrancy during initialization

Lines of code Vulnerability details Impact The solution uses: "@openzeppelin/contracts": "^4.0.0", "@openzeppelin/contracts-upgradeable": "^4.3.2", These dependencies have a known high severity vulnerability: Which makes these contracts vulnerable: contracts/helpers/CryptoPunksHelper.sol: 19:...

Use of deprecated Chainlink oracle aggregator function latestAnswer

Lines of code Vulnerability details Impact According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0. The function is not present in the latest API reference AggregatorInterfaceV3. Proof of Concept...

Wrong calculation for yVault price per share if decimals != 18

Lines of code Vulnerability details The yVault.getPricePerFullShare function calculates the price per share by multiplying with 1e18 token decimals with the assumption that the underlying token always has 18 decimals. yVault has the same amount of decimals as it's underlying token see...

New YVault depositors can be attacked by depressing share decimals

Lines of code Vulnerability details Impact An attacker can become the first depositor for a recently created YVault contract, providing a tiny amount of token by calling deposit1 raw values here, 1 is 1 wei, 1e18 is 1 token if it is 18 decimals. Then the attacker can directly transfer, for exampl...

setDebtInterestApr() doesn't accrue interest before changing it

Lines of code Vulnerability details Impact Wrong interest can be charged if interest is changed without calling accrue Proof of Concept The function setDebtInterestApr updates the interest charged on debt without calling accrue before to compute previous interests. If someone takes debt at 2% and...

Early Depositor can steal funds from subsequent depositors

Lines of code Vulnerability details Impact An early depositor to yVault.sol, preferably the first to deposit, will have the ability to steal funds from subsequent user deposits. The malicious user is able to do this by directly transferring tokens to either the yVault or Controller contracts...

use of deprecated chainlink oracle method

Lines of code Vulnerability details Impact latestanswer , this method does not error if no answer has been reached, it will simply return 0, since we have checks in function requireanswer 0, "invalidoracleanswer"; we may not get the latest value of current price which can affect the functionality...

Controller.setStrategy tries to withdraw JPEG token with incorrect function strategy.withdraw(address), leading to certain revert and renders setStrategy unuseable

Lines of code Vulnerability details Impact Whenever STRATEGISTs want to switch from currently used strategy to another one, they are required to call the Controller.setStrategy function. This function is responsible for first withdrawing CRV and JPEG tokens from strategy contract into vault, then...

[WP-H22] Bad debts should not continue to accrue interest

Lines of code Vulnerability details uint256 debtAmount = getDebtAmountnftIndex; require debtAmount = getLiquidationLimitnftIndex, "positionnotliquidatable" ; // burn all payment stablecoin.burnFrommsg.sender, debtAmount; In the current design/implementation, the liquidator must fully repay the...

[WP-H13] LockPosition can be overwritten by new lockFor() which leads to user's funds loss

Lines of code Vulnerability details function lockFor address account, uint256 nftIndex, uint256 lockAmount external onlyOwner nonReentrant jpeg.safeTransferFromaccount, addressthis, lockAmount; positionsnftIndex = LockPosition owner: account, unlockAt: block.timestamp + lockTime, lockAmount:...

[WP-H5] yVault.sol A malicious early user/attacker can manipulate the vault's pricePerShare to take an unfair share of future users' deposits

Lines of code Vulnerability details function deposituint256 amount public noContractmsg.sender requireamount 0, "INVALIDAMOUNT"; uint256 balanceBefore = balance; token.safeTransferFrommsg.sender, addressthis, amount; uint256 supply = totalSupply; uint256 shares; if supply == 0 shares = amount; el...

JPEGLock.lockFor can relock for specific nft, which overwrites previous lock and cause JPEG loss of the locker

Lines of code Vulnerability details Impact NFTVault contract allows anyone to finalize a pending NFT value proposal by calling the finalizePendingNFTValueETH function. A finalizer must lock an equivalent amount of JPEG tokens to the proposed NFT value in JPEGLock, and can only withdraw those toke...

Deprecated CHAINLINK API usage

Lines of code Vulnerability details Impact Usage of deprecated chainlink function to get collateral price. Proof of Concept The Chainlink API latestAnswer used in the FungibleAssetVaultForDAO contract is deprecated: This method returns the last value but that value cannot be fully updated. New V3...

Oracle Prices Used Could Be Stale Or Manipulated due to latestAnswer() call

Lines of code Vulnerability details Impact In the various calls to the Chainlink oracle, the deprecated API function latestAnswer is used. This approach is vulnerable to price manipulation and stale prices according to the Chainlink documentation. This vulnerability was marked as Medium severity ...

Use of deprecated Chainlink API

Lines of code Vulnerability details Impact The contract uses Chainlink’s deprecated API latestAnswer. Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs. Impact: Deprecated API stops working. Prices cannot be obtained. Protocol stops and contracts have to b...

finalizePendingNFTValue() May Lock an NFT Value Twice Overwriting The Previous Call

Lines of code Vulnerability details Impact If the same NFT index is locked more than once then the initial locker will lose their funds, they will not be recoverable. The function lockFor does not account for the case where the nftIndex is already in use. If this index is already in use...

Use of deprecated oracle API in _collateralPriceUsd

Lines of code Vulnerability details Likelihood low, impact high. The Chainlink latestAnswer function included in IAggregatorV3Interface and called in FungibleAssetVaultForDAOcollateralPriceUsd is considered deprecated and no longer included in the Chainlink API documentation. It's considered best...

StrategyPUSDConvex.balanceOfJPEG uses incorrect function signature while calling extraReward.earned, causing the function to unexpectedly revert everytime

Lines of code Vulnerability details Impact As specified in Convex BaseRewardPool.sol and VirtualRewardPool.sol, the function signature of earned is earnedaddress. However, balanceOfJPEG did not pass any arguments to earned, which would cause balanceOfJPEG to always revert. This bug will propagate...

Usage of deprecated ChainLink API

Lines of code Vulnerability details Impact The Chainlink API latestAnswer function is used in two places but it is deprecated: This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs The latestAnswer function does not revert if no answer has been reached but...

Initial yVault 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 yVault is able to maliciously manipulate the share price by depositing the lowest possible amount 1 wei and then artificially blowing up the yVault token balance. Following depositors will loose their deposited funds due to...

Uncontrolled usage of Chainlink API for core price retrieval

Lines of code Vulnerability details Impact Chainlink's latestAnswer usage can yield stale price information, which is crucial for borrowing and liquidation. latestAnswer is having less ways to be controlled compared to latestRoundData, which is advised for price sensitive operations. Staling pric...

Use of deprecated Chainlink function latestAnswer

Lines of code Vulnerability details function normalizeAggregatorAnswerIAggregatorV3Interface aggregator internal view returns uint256 int256 answer = aggregator.latestAnswer; uint8 decimals = aggregator.decimals; requireanswer 0, "invalidoracleanswer"; //converts the answer to have 18 decimals...

LPFarming, yVault and yVaultLPFarming contract white listing can be surpassed

Lines of code Vulnerability details Impact LPFarming, yVault and yVaultLPFarming contracts' white list checks are performed with isContract and can be surpassed. isContract can only be used for positive confirmations, i.e. filtering out EOAs. Setting high severity as it is a direct access control...

Use latestRoundData instead latestAnswer of Chainlink aggregators

Lines of code Vulnerability details Impact Use latestAnswer in vaults/NFTVault.sol may get stale ETH price in USD ethPriceUSD, stale value in ETH of the NFT ​​getNFTValueETH, stale JPEG price in USD jpegPriceUSD, and also stale USD price of one unit of collateral asset collateralPriceUsd of...

Usage of deprecated Chainlink functions

Lines of code Vulnerability details Impact The Chainlink function latestAnswer is deprecated. Instead, use latestRoundData. As seen in the changelog, Chainlink encourages people to use the latestRoundData function. It's not clear when the support for deprecated functions ends. Here's the same iss...

!_account.isContract() can be bypassed

Lines of code Vulnerability details Impact A contract in construction can bypass isContract to call deposit and withdraw functions in vaults/yVault/yVault.sol, farming/LPFarming.sol, and farming/yVaultLPFarming.sol. Also, Block contracts may cause DoS if someone uses multisig contracts as a calle...

[WP-H9] _swapUniswapV2 may use an improper path which can cause a loss of the majority of the rewardTokens

Lines of code Vulnerability details function harvestuint256 minOutCurve external onlyRoleSTRATEGISTROLE convexConfig.baseRewardPool.getRewardaddressthis, true; //Prevent Stack too deep errors DexConfig memory dex = dexConfig; IERC20 memory rewardTokens = strategyConfig.rewardTokens; IERC20 weth =...