User fund stuck

Judge @GalloDaSballo has assessed the 3rd item in QA Report 26 as Medium risk. The relevant finding follows: … Contract: In withdraw function, Funds will stuck if user deposited a amount and then isDepositordepositor is set to false by Admin. Now user cannot withdraw the amount since onlyDeposito...

Missing validations for return value of oracle data feed.

Lines of code Vulnerability details Impact In ChainlinkUsdWrapper there are no validations for answerthe price if the price is 0 or not. I checked ethOracle0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419..latestRoundData. However, this contract has no validation for the price too. In addition to that,...

Function deposit can receive both ETH and tokens, but only compute tokens

Lines of code Vulnerability details Impact ETH can be transfered to the contract without being computed as a deposit. Proof of Concept The function depositVaultReserve.sol can accept both tokens and ETH. Suppose that Vault accidentally transfer eth and an amount of tokens . The contract will...

EthPool use payable.transfer with an arbitrary receiver

Lines of code Vulnerability details Impact EthPool sends out native tokens via payable.transfer call. This is unsafe as transfer has hard coded gas budget and can fail when the to is a smart contract. Such transactions will fail for smart contract users which don't fit to 2300 gas stipend transfe...

Customers cannot redeem() LP tokens to non-EOA accounts

Lines of code Vulnerability details The use of payable.transfer is heavily frowned upon because it can lead to the locking of funds. The transfer call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfe...

Lack of safeApprove(0) prevents some registrations, and the changing of stakers and LP tokens

Lines of code Vulnerability details OpenZeppelin's safeApprove will revert if the account already is approved and the new safeApprove is done with a non-zero value function safeApprove IERC20 token, address spender, uint256 value internal // safeApprove should only be called when setting an initi...

Oracle data feeds are insufficiently validated

Lines of code Vulnerability details Impact If the oracle price feeds are insufficiently validated, there will be pricing errors leading to the miss-pricing of assets/risk Proof of Concept The code does not verify that answeredInRound = roundID for both cases where an oracle is used, and the...

Swapper3Crv.sol use the wrong address for sushiswap

Lines of code Vulnerability details Impact This seems to be out of scope, but still worth mentioning. This line should be address sushiSwap = SUSHISWAP; --- The text was updated successfully, but these errors were encountered: All reactions...

RoleManager.sol: _revokeRole doesn't remove from the _roleMembers[role] set

Lines of code Vulnerability details Impact In the file RoleManager.sol the function revokeRole doesn't remove the account from the roleMembersrole set. This makes getRoleMemberCount wrong for the else part therefore the renounceGovernance require, on the number of governor, useless and risky as...

CompoundHandler and other contracts call native payable.transfer

Title CompoundHandler topUp, EthPool doTransferOut and VaultReserve withdraw call native payable.transfer Impact Multiple operations are handled by using a payable.transfer call. transfer has a gas budget limit which is unsafe because gas costs can and may change and can fail when the user is a...

_incrementGaugeWeight allows user to add weight to nonexistent gauges

Lines of code Vulnerability details Impact User adds weight to a gauge that hasn't been added In addition to adding to a nonexistent gauge it also increments totalWeight which only contains weight for live gauges. This value then results in returning values for reward distribution that account fo...

If currentMonth in init is 0, then CPI update will revert, zero div

This is a manual upgrade of the sixth item in QA report 86 , per judge @jack-the-pug's assessment of it as a Medium risk issue. If currentMonth in init is 0, then CPI update will revert, zero div --- The text was updated successfully, but these errors were encountered: All reactions...

BufferStored could be larger than BufferCap after _setBufferCap()

Judge @jack-the-pug has assessed the second item in QA Report 64 as Medium risk. The relevant finding follows: Impact In RateLimited.sol BufferCap should be the upper bound of BufferStored, However in setBufferCap it calls updateBufferStored before replacing the old BufferCap. If old BufferCap is...

AmmGauge stake allows for reentrancy that can lead to stealing the contract balance

Lines of code Vulnerability details Impact Some ERC20 do allow for user's control of execution. For example, ERC777 has tokensReceived hook. This way, an ability to reenter can be executed with the usage of any such tokens. AmmGauge stake do not control for reentrancy and uses balance difference ...

[WP-H15] AmmConvexGauge.sol#poolCheckpoint() cvxStakedIntegral can be manipulated by the attacker

Lines of code Vulnerability details function poolCheckpoint public virtual override returns bool if killed return false; uint256 timeElapsed = block.timestamp - uint256ammLastUpdated; uint256 currentRate = IControllercontroller.inflationManager.getAmmRateForToken ammToken ; uint256 crvEarned =...

[WP-H0] xERC4626.sol Some users may not be able to withdraw until rewardsCycleEnd the due to underflow in beforeWithdraw()

Lines of code Vulnerability details function beforeWithdrawuint256 amount, uint256 shares internal virtual override super.beforeWithdrawamount, shares; storedTotalAssets -= amount; function syncRewards public virtual uint192 lastRewardAmount = lastRewardAmount; uint32 timestamp =...

Incorrect accounting of free weight in _decrementWeightUntilFree

Lines of code Vulnerability details Impact In decrementWeightUntilFree, the free weight is calculated by balanceOfuser - getUserWeightuser plus weight freed from non-deprecated gauges. The non-deprecated criteria is unnecessary and lead to incorrect accounting of free weight. Proof of Concept...

ETHVault, BkdEthCvx and VaultReserve use payable.transfer for inter-system transfers

Lines of code Vulnerability details Impact These contracts use payable.transfer for internal fund transfer, where the recipients are vaults, pools, strategies. This is generally unsafe as transfer has hard coded gas budget and can fail when the to is a smart contract. Such transactions will fail...

Malicious Stakers can grief Keepers

Lines of code Vulnerability details Impact A Staker -- that has their top-up position removed after execute is called by a Keeper -- can always cause the transaction to revert. They can do this by deploying a smart contract to the payer address that has implemented a receive function that calls...

Swapper3Crv's swapping path can be suboptimal

Lines of code Vulnerability details Impact Swapper3Crv.swap result can be suboptimal as only paths with ETH are evaluated. Setting severity to medium as despite function availability not affected there can be some fund losses as a result. Proof of Concept tokenAmountOut uses fixed tokenIn, ETH,...

CvxCrvRewardsLocker implements a swap without a slippage check that can result in a loss of funds through MEV

Lines of code Vulnerability details Impact The CvxCrvRewardsLocker contract swaps tokens through the CRV cvxCRV pool. But, it doesn't use any slippage checks. The swap is at risk of being frontrun / sandwiched which will result in a loss of funds. Since MEV is very prominent I think the chance of...

setFlywheelRewards can take any rewardToken

Lines of code Vulnerability details Impact Though setFlywheelRewards has requiresAuth, it still has rug risk that a privileged user can move all rewardToken of flywheelRewards to new malicious newFlywheelRewards unconditionally. Proof of Concept A malicious user or a compromised admin can call...

Customers cannot be topUp()ed a second time

Lines of code Vulnerability details OpenZeppelin's safeApprove will revert if the account already is approved and the new safeApprove is done with a non-zero value function safeApprove IERC20 token, address spender, uint256 value internal // safeApprove should only be called when setting an initi...

First xERC4626 deposit exploit can break share calculation

Lines of code Vulnerability details Solmate convertToShares function follow the formula: assetDepositAmount totalShareSupply / assetBalanceBeforeDeposit. The share price always return 1:1 with asset token. If everything work normally, share price will slowly increase with time to 1:2 or 1:10 as...

Division by zero in isWithinDeviationThreshold

Judge @jack-the-pug is upgrading the following issue from a QA report issue 30 to Medium risk: Division by zero in isWithinDeviationThreshold if a is zero. This only seems to be the case if the oracle would return 0 for CPI and in this case, something is wrong anyway. Should still handle this err...

ChainlinkOracleProvider can provide zero and stale prices

Lines of code Vulnerability details Impact As stale price is determined by time since last timestamp, the price that is most recent, but wasn't updated for more than 2 hours say there were no trades on the market will be rejected, which makes system functionality unavailable in such a case. This...

first depositor 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 Alice is the first one to deposit in LiquidityPool. she deposits 1 basic unit of the token, therefore minting one lp token...

Unsafe use of .transfer

Lines of code Vulnerability details Impact User funds can be locked by using a smart contract wallet with inefficient callback, or by future hardforks that change the gas consumption. address.transfer has been suggested to deprecate by most auditors, because of the potential OOG error. However,...

_revokeRole doesn't remove account from roleMember set

Lines of code Vulnerability details Impact The function doesn't remove the address from roleMembersrole set, which will mess up with the roleCount Proof of Concept Tools Used Recommended Mitigation Steps rolesrole.membersaccount = false; roleMembersrole.removeaccount; --- The text was updated...

FlywheelCore's setFlywheelRewards can remove access to reward funds from current users

Lines of code Vulnerability details Impact FlywheelCore.setFlywheelRewards can remove current reward funds from the current users' reach as it doesn't check that newFlywheelRewards' FlywheelCore is this contract. If it's not, by mistake or with a malicious intent, the users will lose the access t...

Unlimited reward minting with Function Transfer in StakerVault (updates balances before calling userCheckpoint)

Lines of code Vulnerability details Impact The bug in "StakerVault.transfer" function which is externally callable is that first it is updating the balance of sender and receiver then it calls ILpGaugelpGauge.userCheckpoint for those addresses. Function userCheckpoint use balance of address to...

function lockFunds in TopUpActionLibrary can cause serious fund lose. fee and Capped bypass. It's not calling stakerVault.increaseActionLockedBalance when transfers stakes.

Lines of code Vulnerability details Impact In function TopUpActionLibrary.lockFunds when transfers stakes from payer it doesn't call stakerVault.increaseActionLockedBalance for that payer so stakerVault.actionLockedBalancespayer is not get updated for payer and...

FlywheelCore.setBooster() can be used to steal unclaimed rewards

Lines of code Vulnerability details Impact A malicious authorized user can steal all unclaimed rewards and break the reward accounting Even if the authorized user is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. Furthermore since this...

User can steal all rewards due to checkpoint after transfer

Lines of code Vulnerability details Impact I believe this to be a high severity vulnerability that is potentially included in the currently deployed StakerVault.sol contract also. The team will be contacted immediately following the submission of this report. In StakerVault.sol, the user...

In ERC20Gauges, contribution to total weight is double-counted when incrementGauge is called before addGauge for a given gauge.

Lines of code Vulnerability details Impact The impact depends really on how gauges are used by other contracts. The most obvious consequence I can imagine is that some other contract distributes rewards based on calculateGaugeAllocation. However, because getStoredWeighttotalWeight, currentCycle i...

transfer or transferFrom without checking the boolean result

It was found some transfer, approve or transferFrom without checking the boolean result, ERC20 standard specify that the token can return false if this call was not made, so it's mandatory to check the result of approve methods. CoreCollection.solL175 ERC721Payable.solL54 --- The text was updated...

Unsafe ERC20 transfer Operations

Findings Unsafe ERC20 transfer Operations The transfer and transferFrom functions return a Boolean value which should be checked for successful transfer, Some tokens do not revert if the transfer did fail but return false. the protocol Joyn do have some lines for code that make a use of these...

Don't ignore ERC20 transfer return values

You're ignoring the return value of an ERC20 transfer twice: Either use SafeERC20 or check the return value as you do in other places in the code base. --- The text was updated successfully, but these errors were encountered: All reactions...

use safe erc20 functions

ERC721Payable.handlePayment core-contracts/contracts/ERC721Payable.sol50-56 ignores return value by payableToken.transferFrom core-contracts/contracts/ERC721Payable.sol54 consider safeTransferFrom to prevent any locks or loss of funds --- The text was updated successfully, but these errors were...

Transfer return value is ignored

Impact Some ERC20 tokens, such as USDT, don't revert when transfer/transferFrom fails. The transfer return value has to be checked as there are some other tokens that returns false instead revert. safeTransfer should be used instead of transfer Proof of Concept safeTransferFrom should be used...

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

Price can be stale leading to wrong return value in refreshedAssetPerBaseInUQ in ChainlinkPriceOracle.sol

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. Stale price from data feed can lead to incorrect value of assetPerBaseInUQ Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that...

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

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

Insufficient oracle data feed validation

Lines of code Vulnerability details Impact Stale prices can lead to the incorrect valuation of assets Proof of Concept The code does not check the other data returned from latestRoundData which must be used to ensure that the data is not stale and that the price is valid File:...

Chainlink's latestRoundData might return stale or incorrect results

Lines of code Vulnerability details , int basePrice, , , = baseAggregator.latestRoundData; On ChainlinkPriceOracle.sol, we are using latestRoundData, but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:...

Wrong shareChange() function (vToken.sol)

Lines of code Vulnerability details Impact Users can get the wrong amount of vToken = Make users lose their fund Proof of Concept Base on the code in function shareChange in vToken.sol Assume that if oldShare = totalSupply 0, newShares = amountInAsset totalSupply - oldShares / assetBalance -...

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