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

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

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

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

Users wont be able to withdraw staked tokens since vesting function is called differently in interface and implementation

Lines of code Vulnerability details Impact Users can't withdraw their funds Proof of Concept When withdrawing in staking the contract will send funds to vesting using setupVesting. This is defined in the vesting interface. On the other hand, the vesting implementation calls this function vest so...

Race between governance and strategist on other token earned

Lines of code Vulnerability details Impact There is a race between the strategist and the governance to report other tokens earned by the strategy. Indeed the strategist can trigger the function 1 by calling the strategy while the governance can call 2. Both these functions can report earn tokens...

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

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

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

Staked Citadel function balance and reportHarvest

Lines of code Vulnerability details Impact In the StakedCitadel...

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Use of deprecated oracle API in _normalizeAggregatorAnswer

Lines of code Vulnerability details Use of deprecated oracle API in normalizeAggregatorAnswer Likelihood low, impact high. The Chainlink latestAnswer function included in IAggregatorV3Interface and called in NFTVaultnormalizeAggregatorAnswer is considered deprecated and no longer included in the...

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

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

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

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

JPEGLock lockups overwrite previous data

Lines of code Vulnerability details Impact If a user calls NFTVault.finalizePendingNFTValueETH a second time without first calling JPEGLock.unlock to recover their previous lockup, their balance will be overwritten leaving the previous lockup balance unrecoverable. Proof of Concept POC by adding...

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

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

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