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

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

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

Shelter withdraw does not check if user already claimed

Lines of code Vulnerability details Impact Shelter withdraw does not check if user already claimed. This allow any user with non-zero claim to drain the Shelter. Proof of Concept function withdrawIERC20 token, address to external override requireactivatedtoken != 0 && activatedtoken + GRACEPERIOD...

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

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

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

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

Shelter funds can be stolen

Lines of code Vulnerability details Impact Shelter has a function withdraw that lets whitelisted users withdraw a specified amount of some token. The function does not check if the user has already withdrew the tokens. Since, a user can withdraw allowed amount any number of times, stealing all th...

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

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

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

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

Shelter sets wrong claimed field

Lines of code Vulnerability details The Sheler.withdraw function sets the claimedtokenuser field but uses the shares of msg.sender. An attacker can withdraw tokens several times passing different to addresses, each time the msg.sender's shares will be used to receive tokens. function withdrawIERC...

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

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

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

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

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

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

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

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

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

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

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

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

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

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

All Tokens Can Be Stolen From Shelter Contract

Lines of code Vulnerability details Impact function withdrawIERC20 token, address to external override requireactivatedtoken != 0 && activatedtoken + GRACEPERIOD block.timestamp, "shelter not activated"; uint256 amount = savedTokenstoken client.shareOftoken, msg.sender / client.totalSharetoken;...

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

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

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

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

Owner can steal input tokens

Lines of code Vulnerability details Impact Owner is in full control over the saleRecipient address. When a buy transaction enters the mempool, an owner can frontrun the buy with a transaction that calls setTokenOutPrice and sets the price to a very high value, effectively making bought tokens clo...

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

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

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

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

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

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

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

Sale should be able to be automatically finalized

Lines of code Vulnerability details Impact The admins are the only ones that are able to finalize the sale. Throughout the token buying process, the admins are transferred tokenIn tokens, and the users are expecting to be able to get the corresponding tokenOut tokens once the sale is finalized. T...

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

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

Owner never calls finalize() = rug pull

Lines of code Vulnerability details Impact In order for users to claim their promised tokenOut tokens, the contract owner must call the finalize function. If the owner never calls the finalize function, no user can call the claim function to get their tokens. The owner can call the sweep function...

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

Sale participants’ claims are uncollateralized until the sale finalization

Lines of code Vulnerability details Impact Sale participants have to bear the full risk of owner misbehavior or just a technical malfunction until the very end of the sale. For example, if owner's private key is somehow lost, all participants deposits to be gone if tokenOut isn't yet transferred ...

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

Zero tokenOut balance = rug pull

Lines of code Vulnerability details Impact The only time that the Badger Citadel contract checks that the balance of tokenOut is greater than or equal to totalTokenOutBought is in the finalize function, which happens at the end of the sale. A contract owner can start a token sale but never send...