Shelter.claim does not check if already claimed

Lines of code Vulnerability details The Sheler.withdraw function sets the claimedtokenuser field but does not check if the user is allowed to claim by checking require!claimedtokenuser, "already claimed". function withdrawIERC20 token, address to external override requireactivatedtoken != 0 &&...

claimed of _to set to true

Lines of code Vulnerability details Impact Function withdraw in Shelter calculates the amount based on shares of msg.sender, but sets the claimed flag of to: uint256 amount = savedTokenstoken client.shareOftoken, msg.sender / client.totalSharetoken; claimedtokento = true; This means a malicious...

double spend in Shelter

Lines of code Vulnerability details in the withdraw function in Shelter, one can withdraw infinite times, and by doing so, draining the system. the function doesn't check that the shares aren't already withdrawn. --- The text was updated successfully, but these errors were encountered: All reacti...

Wrong reward token calculation in MasterChef contract

Lines of code Vulnerability details Impact When adding new token pool for staking in MasterChef contract function addaddress token, uint allocationPoints, uint16 depositFee, uint startBlock All other, already added, pools should be updated but currently they are not. Instead, only totalPoints is...

Imbalance and LP gain is forever stuck in USDMPegRecovery

Lines of code Vulnerability details Impact After depositing into Curve, underlying USDM and pool3 of each LP share will change due to 1. Swap fee 2. Pool imbalance However, userLiquidity does not update according to current underlying LP balance. When guardian remove liquidity from Curve, at leas...

Remaining reward balance is wrongly updated

Lines of code Vulnerability details The ConvexStakingWrapper.calcRewardIntegral function makes the dreward = IERC20reward.token.balanceOfaddressthis; - reward.remaining amount available for claiming. Then it updates the reward.remaining value to the balance before the distribution. RewardType...

Owner can steal Concur rewards

Lines of code Vulnerability details Impact Owner can steal Concur rewards by adding a depositor and inflating other depositors' assigned balance of the token within the contract. Thus, the owner-managed depositor can get most all but one wei of the created tokens. Tools Used Manual analysis...

Deposits after the grace period should not be allowed

Lines of code Vulnerability details Impact Function donate in Shelter shouldn't allow new deposits after the grace period ends, when the claim period begins. Otherwise, it will be possible to increase savedTokenstoken, and thus new user claim amounts will increase after some users might already...

[WP-H5] ConvexStakingWrapper.sol#withdraw() Use of non-existing method on convexPool[_pid] makes withdraw() always reverts

Lines of code Vulnerability details function withdrawuint256 pid, uint256 amount external nonReentrant WithdrawRequest memory request = withdrawRequestpidmsg.sender; requirerequest.epoch = amount, "too much"; checkpointpid, msg.sender; depositspidmsg.sender.amount -= uint192amount; if amount 0...

Same reward token in pools can break accounting

Lines of code Vulnerability details The ConvexStakingWrapper contract uses several reward pool tokens rewardspidindex.token and it can be that the same token is used for different pids. Indeed, the CVX/CRV tokens are always at index 0 and 1. The rewards will be distributed to the first pool id pi...

USDMPegRecovery does not account for fees

Lines of code Vulnerability details Curve.fi pools charge fees when adding or removing liquidity. The only time fees are not applied are when withdrawals are done using removeliquidity. USDMPegRecovery keeps track of tokens deposited and withdrawn, but does not keep track of fees assessed on thes...

Potential for reentrancy on USDMPegRecovery.sol:withdraw()

Lines of code Vulnerability details Impact Re-entrancy Proof of Concept File: USDMPegRecovery.sol 110: function withdrawLiquidity calldata withdrawal external 111: Liquidity memory total = totalLiquidity; 112: Liquidity memory user = userLiquiditymsg.sender; 113: ifwithdrawal.usdm 0 114:...

Unconstrained fee

Lines of code Vulnerability details Impact Token fee in MasterChef can be set to more than 100%, for example by accident causing all deposit calls to fail due to underflow on subtraction when reward is lowered by the fee, thus breaking essential mechanics. Note that after the fee has been set to...

MasterChef.add must update pools first

Lines of code Vulnerability details The MasterChef.add function changes the total pool allocation but does not update other pools first. When other pools are finally updated at some point, then accConcurPerShare will be wrongly computed with their smaller allocPoint / newTotalAllocPoint share eve...

missing check on claimed token

Lines of code Vulnerability details Impact the withdraw can be used by user to claim a token, however this function didnt check of the user had already claimed or not, therefore setting claimedtokento = true; is useless, therefore the user can claimed in multiple times, till the contract is...

Re-entrancy vulnerabilities

Lines of code Vulnerability details Impact Function claimRewards in ConcurRewardPool should be re-entrancy protected or first nullify the reward before sending it, otherwise, if any token contains a transfer callback hook, users can claim the same rewards multiple times, by re-entering the functi...

safeConcurTransfer can cause loss of funds

Lines of code Vulnerability details Impact All calculations are rounded down, since a lack of tokens in the contracts cannot be rounding errors' fault. So the function is redundant. On the other hand, if the contract is undersupplied with Concur tokens, this will cause depositors to be sent less...

unsafe cast can lead to theft

Lines of code Vulnerability details in the first link I provided, a hacker can call withdraw with amount = 2196, the amount of shares that will be burned is uint1922192 == 0. if the system has enough money, the hacker can steal 2192 tokens and pay nothing. for the second link, a user that provide...

Potential Re-entrancy Attack via ETH or ERC777 Token Transfer

Lines of code Vulnerability details Impact The CEI pattern is not being implemented properly in the claimRewards function of the ConcurRewardPool.sol. function claimRewardsaddress calldata tokens external override for uint256 i = 0; i tokens.length; i++ uint256 getting = rewardmsg.sendertokensi;...

ConvexStakingWrapper deposits and withdraws will frequently be disabled if a token that doesn't allow zero value transfers will be added as a reward one

Lines of code Vulnerability details Impact If deposits and withdraws are done frequently enough, the reward update operation they invoke will deal mostly with the case when there is nothing to add yet, i.e. reward.remaining match the reward token balance. If reward token doesn't allow for zero...

Reentrancy in ConcurRewardPool::claimRewards

Lines of code Vulnerability details Impact Any address that has nonzero reward for a token tokensi is able to drain all contact token funds if the transfer function is reentrant for example, ERC777 token. As tokensi is an arbitrarily implemented, a reentrant transfer function can be assumed to be...

[WP-H13] MasterChef.sol Users won't be able to receive the concur rewards

Lines of code Vulnerability details According to: README Implementation of deposit: /contracts/MasterChef.solL157-L180 MasterChef is only recording the deposited amount in the states, it's not actually holding the depositToken. depositToken won't be transferred from msgSender to the MasterChef...

Block miscalculations

Lines of code Vulnerability details Impact The updatePool function in MasterChef is supposed to account for all blocks before endBlock. But the if statement checks if the current block is after the deadline. Thus blocks between pool.lastRewardBlock and endBlock won't be accounted for iff...

During stake or deposit, users would not be rewared the correct Concur token, when MasterChef has under-supply of it.

Lines of code Vulnerability details Impact During stake or deposit, users would not be transferred the correct Concur token, when MasterChef has under-supply of it. There is an assumption that MasterChef contract would own enough Concur tokens so as to distribute to users as reward, during deposi...

deposit wrong implementation

Lines of code Vulnerability details the user is the user of msg.sender, which is always the StakingRewards contract, instead of recepient. one of the problems that are caused by this is that anyone who deposits, will get the rewards of all the other users, because everyone shares the same user...

Wrong pools reward calculation. User will get smaller rewards (always)

Lines of code Vulnerability details Impact When adding new token pool for staking in MasterChef contract function addaddress token, uint allocationPoints, uint16 depositFee, uint startBlock All other, already added, pools should be updated but currently they are not. Instead, only totalPoints is...

ConvexStakingWrapper._calcRewardIntegral() Has An Accounting Error When Updating reward.remaining

Lines of code Vulnerability details Impact The ConvexStakingWrapper.sol implementation makes several modifications to the original design. One of the key changes is the way rewards are distributed to stakers. A new ConcurRewardPool.sol contract is used to store rewards, allowing users to claim...

[WP-H8] ConvexStakingWrapper.sol#_calcRewardIntegral Wrong implementation can disrupt rewards calculation and distribution

Lines of code Vulnerability details uint256 bal = IERC20reward.token.balanceOfaddressthis; uint256 dreward = bal - reward.remaining; // send 20 % of cvx / crv reward to treasury if reward.token == cvx || reward.token == crv IERC20reward.token.transfertreasury, dreward / 5; dreward = dreward 4 / 5...

Repeated Calls to Shelter.withdraw Can Drain All Funds in Shelter

Lines of code Vulnerability details Impact tl;dr Anyone who can call withdraw to withdraw their own funds can call it repeatedly to withdraw the funds of others. withdraw should only succeed if the user hasn't withdrawn the token already. The shelter can be used for users to withdraw funds in the...

Repeated withdrawals from Shelter

Lines of code Vulnerability details Impact function withdraw in Shelter sets claimed flag: claimedtokento = true; but it never actually checks if the user has already claimed, so users can invoke the withdrawal function multiple times and claim more rewards than were entitled. Recommended...

[WP-H14] ConvexStakingWrapper, StakingRewards Wrong implementation will send concur rewards to the wrong receiver

Lines of code Vulnerability details UserInfo storage user = userInfopidmsgSender; updatePoolpid; ifuser.amount 0 uint pending = user.amount pool.accConcurPerShare / concurShareMultiplier - user.rewardDebt; if pending 0 safeConcurTransferrecipient, pending; ConvexStakingWrapper, StakingRewards is...

Users’ staking rewards can get redeemed by other users

Lines of code Vulnerability details Can be medium or high: When a user deposits through ConvexStakingWrapper, the contract calls the MasterChef's deposit function and deposits to it with recipient = msg.sender, but the MasterChef actually updates the deposited amount of the ConvexStakingWrapper...

Owner can lock tokens in MasterChef

Lines of code Vulnerability details Impact Owner can remove a depositor. Since only depositors can deposit and withdraw, the owner may add a contract to the whitelist, let users deposit in the contarct and remove the depositor from the whitelist. Depositor's reward cannot be withdrawn then. And...

Option to have setTokenOutPrice, will impact Token sale and user behavior

Lines of code Vulnerability details There is no loss of funds, but there is a possibility that the current code will dictate user behavior which is not intended. Hence setting the risk rating as medium. Impact There is an option to setTokenOutPrice by the owner. There can be two cases. case-1. th...

[WP-H2] Funds can be frozen when critical key holders lose access to their keys

Lines of code Vulnerability details The current implementation requires trusted key holders Owner to send transactions finalize to finalize the sale before the buyers can claim the tokenOut from the contract. function finalize external onlyOwner require!finalized, "TokenSale: already finalized";...

Sale operator may frontrun purchases in order to modify the price arbitrarily

Lines of code Vulnerability details Impact Buyer's cannot set a minimum amount of CTDL that they will expect so BadgerDAO can force them to receive a smaller amount than expected. Proof of Concept The buy function only allows the buyer to specify an amount of tokenIn to send to the contract and...

Possible rug #2

Lines of code Vulnerability details Impact The finalize function can be called only by the owner. Thus, if an owner account is lost, unable to send a transaction, or controlled by a bad actor, the contract may not get finalized. Thus the claim function will never be able to succeed and funds will...

Seven ways in which the Owner and Proxy Admin can make users lose funds ("rug vectors")

Lines of code Vulnerability details The contest explicitly asks to analyze the contract for "Rug Vectors", so that is what this issue is about. note to reviewers This issue list maybe 7 different problems and recommends different fixes. I could have made seven separate issues for each, but it wou...

tokenOutPrice can be updated to rug buyer

Lines of code Vulnerability details Impact The setTokenOutPrice function allow owner to update the tokenOutPrice. The owner of the contract can potential "rug" users by front-runing buy transactions to set a very high price. Since the README file specified "Rug Vectors" in the scope, while this...

tokenOut is only required on finalize

Lines of code Vulnerability details Impact I thought of a potential rug pull from the owner: when users buy tokenOut, it is not required that the contract has already escrowed enough tokenOut. It is only required when finalizing the sale: require tokenOut.balanceOfaddressthis = totalTokenOutBough...

Users should be allowed to control accepted tokenOutPrice

Lines of code Vulnerability details Impact Users should be able to control the accepted price. The owner can anytime invoke function setTokenOutPrice and thus change the ratio of token in/out. Users have to trust the owner not to front-run them and make the tokens more expensive. Recommended...

No guarantee sale organizer will fulfil their end of the deal

Lines of code Vulnerability details Impact Sale participants will only be able to claim their CTDL tokens once the sale is finalized. However, there is no guarantee that it ever will be, because: Sale finalisation can only be performed by the owner The owner is able to change the sale parameters...

Sale recipient can be changed mid-sale

Lines of code Vulnerability details Impact The setSaleRecipient function can change the sale recipient while a sale is open. This function can be only called by the owner of the contract, however, since: 1. the saleRecipient is the address that receives all the input tokens deposited by users, an...

Sales end time can be postponed indefinitely

Lines of code Vulnerability details Impact Both setSaleStart and setSaleDuration allow owner to postpone the end time of a sale, which make it possible for a token sales be lengthened indefinitely. In such case buyer will never be able to claim. Since the README file specified "Rug Vectors" in th...

TokenIn can be swept if saleRecipient == tokenSale

Lines of code Vulnerability details The initialize and setSaleRecipient function allows setting the saleRecipient to the contract itself, which means the tokenIn will remain in the contract. It can then be transferred out by calling sweep. This condition described in sweep does not hold anymore...

No minOutAmount amount checks when buying

Lines of code Vulnerability details The tokenOutPrice can be changed with the setTokenOutPrice even if the sale is already running. Users might accept the current token price, send a purchase transaction, and before it is mined the token price can be un-intentionally changed. The user might recei...

Possible rug #3

Lines of code Vulnerability details Impact Tokens can be consumed by the contract and sent to the saleRecipient address without any guarantee that sale tokens will ever be deposited in the sale contract. The sold tokens are only required in the finalize function, which clearly does not provide an...

Owner could frontrun with higher price

Lines of code Vulnerability details Impact User can buy tokenOut at price X and be front-run by the owner by setting a higher price. Proof of Concept Recommended Mitigation Steps Changes in price by the owner should have some time lock to protect the buyer from frontrunning --- The text was updat...

Improper Upper Bound Definition On The OnlyOwner Function Variables

Lines of code Vulnerability details Impact During the documentation of the contest, It has been seen that the following comment added. Specific care should be put in: Economic exploits Rug Vectors However, onlyOwner function does not have upper bound definition on the related variables. Values th...

[WP-H3] saleRecipient can rug buyers

Lines of code Vulnerability details In TokenSaleUpgradeable.solbuy, tokenIn will be transferred from the buyer directly to the saleRecipient without requiring/locking/releasing the correspoining amount of tokenOut. This allows the saleRecipient to rug the users simply by not transferring tokenOut...