Missing ERC20 return value check in MerkleVesting#withdraw

Lines of code Vulnerability details MerkleVestingwithdraw does not check the return value of the token withdrawal on line 173. If an ERC20 token returns false to indicate a failed transfer but does not revert, this transfer will silently fail but the withdrawal amount will still be deducted from...

setGlobalTax() Can Be Manipulated By The Global Beneficiary To Steal Reward Tokens Or Censor Pool Creators

Lines of code Vulnerability details Impact Upon pool creation, the pool configures a taxPerCapita value which is controlled by the global beneficiary. This global beneficiary account can effectively sandwich calls to addPool by increasing the fee to 100% before the addition of a new pool and...

Users Can Prevent Excess Tokens From Being Withdrawn By The Pool Creator In withdrawExcessRewards()

Lines of code Vulnerability details Impact Because pools will likely never be fully utilised by stakers while active, the following assumption in withdrawExcessRewards can be broken by preventing any receipt withdrawal: requirepool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are...

Protocol unusable for some ERC20 tokens (bad tokens)

Lines of code Vulnerability details Impact Protocol unusable for some ERC20 tokens bad tokens PoC Some tokens don't correctly implement the EIP20 standard and their transfer/transferFrom return void The so called bad tokens. More info This makes the transaction revert when calling. Recommended Us...

decimalMultiplier assumes tokens have <= 18 decimals

Originally submitted by warden StyxRave in 153, duplicate of 49. BkdTriHopCvx.sol decimalMultiplier assumes tokens have = 18 decimals. Will always be 0 for tokens with more than 18 which would be still ERC20 compliant. --- The text was updated successfully, but these errors were encountered: All...

if user send uninitialized poolId to function deposit() of PermissionlessBasicPoolFactory, then attacker can cause user fund to be locked forever, and only unlock it if user pays ransom

Lines of code Vulnerability details Impact Function deposit of PermissionlessBasicPoolFactory supposed to revert if user send uninitialized poolId by mistake, but if user does this, attacker can perform front-running attack and create multiple pools with his smart contract and be owner of that...

Excess ETH is not returned to sender

Lines of code Vulnerability details Impact In passThruGate function, msg.value is checked to be greater than the required cost, but the excess amount is not returned to the sender. Proof of Concept function passThruGateuint index, address override external payable Gate memory gate = gatesindex;...

Missing freshness validation in ETH price oracle

Originally submitted by warden horsefacts in 199, duplicate of 17. Missing freshness validation in ETH price oracle The ChainlinkUsdWrapperethPrice function does not check for a nonzero answer or validate that the price was returned in a recent round: ChainlinkUsdWrapperethPrice function ethPrice...

Users can not initialize and withdraw tokens if coinsPerSecond is 0

Lines of code Vulnerability details Impact If a user tries to claim a few totalCoins with a long vestingTime, this user will call the initialize function failed, and can not withdraw funds. Proof of Concept In MerkleResistor.sol L259: uint coinsPerSecond = totalCoins uint100 - tree.pctUpFront /...

Bogus deposits in Pools possible

Lines of code Vulnerability details Impact Bogus deposits in the Pools is possible by calling the function deposit with a malicious ERC20 token that always returns true whenever transferFrom is called. Tools Used Manual Inspection Recommended Mitigation Steps Create a whitelist of allowed ERC20...

In MerkleVesting.sol::withdraw check return value of ERC20 transfer or use safeTransfer of OZ

Lines of code Vulnerability details Impact MerkleVesting.solL173 tree.tokenBalance -= currentWithdrawal; IERC20tree.tokenAddress.transferdestination, currentWithdrawal; In case of failed transfer here it do not check return value of transfer. it updates the tree balance without transfering the...

Avoid payable.transfer

Originally submitted by warden horsefacts in 199, duplicate of 52. Avoid payable.transfer EthPool and EthVault both use payableaddress.transfer to transfer ETH. It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed amount of gas and may revert if futur...

PermissionlessBasicPoolFactory use hard coded decimals of 18

Lines of code Vulnerability details Once reward/deposit tokens decimals differ from 18 the calculations with a hard coded 1e18 will become grossly incorrect. This will lead either to receiving no rewards: say deposit is USDC with decimals of 6, being divided by 1e18 it adds 1e-12 to the rewards...

Not all ERC20 tokens return boolean on transfer

Lines of code Vulnerability details Impact Some ERC20 tokens do not conform to the standard of returning a boolean when transfer is called. If one of these tokens is included as a reward token, the withdraw function will be irrevocably broken, and users won't be able to collect their reward or...

Fee-On-Transfer Tokens Are Not Supported

Lines of code Vulnerability details Impact The FactoryDAO suite of contracts interact with any arbitrary ERC20 token. Because of this, there is a specific instance and likely several others where a fee-on-transfer token will not be correctly handled. PermissionlessBasicPoolFactory.fundPool will...

transfer is used for transfering ether

Originally submitted by warden pauliax in 173, duplicate of 52. .transfer is used for transfering ether, e.g.: payableto.transferamount; payablemsg.sender.transferamount; It is currently not recommended as recipients with custom fallback functions smart contracts will not be able to handle that...

If A User Mistakenly Provides Too Much Ether To The passThruGate() Function, This Additional Amount Will Be Forever Locked Within The Contract

Lines of code Vulnerability details Impact The passThruGate function acts as a proxy function to the beneficiary address by attaching Ether to the call. If an excess of Ether is provided to the call, only gate.ethCost will be sent to the beneficiary. Excess Ether will be forever be locked in the...

Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies

Lines of code Vulnerability details Impact Wrong bookkeeping, albeit limited to the concerned tree with a FoT Token Wrong amount emitted Proof of Concept contracts/MerkleDropFactory.sol: 77: requireIERC20merkleTree.tokenAddress.transferFrommsg.sender, addressthis, value, "ERC20 transfer failed";...

FixedPricePassThruGate locked ether

Lines of code Vulnerability details Impact Contract FixedPricePassThruGate is a pass thru gate that is passing funds to the gate's beneficiary. Function passThruGate requires to send ether that is equal or more than gate.ethCost. In the case of receiving more ether than gate.ethCost, passThruGate...

steal user funds with front-running when he calls depositTokens() of MerkleVesting and MerkleResistor with wrong treeIndex (uninitiated)

Lines of code Vulnerability details Impact This nature of this bug is similar in MerkleVesting and MerkleResistor and MerkleDropFactory, so I only write MerkleDropFactory version: If a user calls depositTokens with wrong treeIndex value by mistake, attacker can perform front-running attack and...

SpeedBumpPriceGate does not refund excess ETH payment

Lines of code Vulnerability details The FixedPricePassThruGate accepts ETH amounts greater than or equal to the calculated price, and forwards the full amount to the gate's configured beneficiary address. However, there is no mechanism to refund these excess payments, and no guarantee that the...

FixedPricePassThruGate.sol All the msg.value should be pass thru to gate.beneficiary instead of gate.ethCost

Lines of code Vulnerability details In FixedPricePassThruGate.solpassThruGate, at L48 the msg.value is checked to be = gate.ethCost instead of == gate.ethCost, which makes it possible for the caller to send more than gate.ethCost. However, at L53 only the amount of gate.ethCost is passed thru to...

Reward Token Transfer Failure Can Lead to Loss of Deposit in PermissionlessBasicPoolFactory

Lines of code Vulnerability details Impact If transfer of any reward token returns False or reverts for whatever reason, users who deposited will not be able to withdraw their deposit. A malicious pool creator could abuse this to lock tokens from victims by using two reward tokens, one...

Forget to remove account out of _roleMembers[role]

Originally submitted by warden TrungOre in 89, duplicate of 164. For get to remove account out of roleMembersrole --- The text was updated successfully, but these errors were encountered: All reactions...

ChainLink latestRoundData data may be stale

Originally submitted by warden 0xkatana in 63, duplicate of 17. ChainLink latestRoundData data may be stale Impact The Chainlink API latestRoundData function returns price data with other timestamp and round data. The timestamp and round data should be validated to confirm the data is not stale...

PermissionlessBasicPoolFactory.sol Does Not Support Reward Tokens With Decimals Other Than 18

Lines of code Vulnerability details Impact The PermissionlessBasicPoolFactory.sol contract allows anyone to add staking pools which users can participate in to earn reward tokens. Pools are segregated to ensure malicious pools cannot siphon tokens from honest pools. Upon the addition of a new poo...

PermissionlessBasicPoolFactory.sol Does Not Support Reward Tokens With Decimals Other Than 18

Lines of code Vulnerability details Impact The PermissionlessBasicPoolFactory.sol contract allows anyone to add staking pools which users can participate in to earn reward tokens. Pools are segregated to ensure malicious pools cannot siphon tokens from honest pools. Upon the addition of a new poo...

if user call addMerkleTree() of MerkleIdentity with priceIndex==0 by mistake or other uninitialized gates, then attacker can steal all the NFTs with 0 payment

Lines of code Vulnerability details Impact If treeAdder calls addMerkleTree with priceIndex==0 by mistake, then attackers can buy NFT's with price of 0 because gate Indexs in FixedPricePassThruGate or SpeedBumpPriceGate start from 1 and gates0 will be uninitialized and getCost0 will return 0. Thi...

Missing a storage slot

Lines of code Vulnerability details Impact By using a pre-increment in our instantiations, we are potentially missing out on using the first 0th slot. Tools Used Manual Inspection Recommended Mitigation Steps Use post-incement --- The text was updated successfully, but these errors were...

PermissionlessBasicPoolFactory's pools with fee on transfer tokens can be emptied by repetitive deposit-withdraws

Lines of code Vulnerability details Griefing attack is possible if pool deposit token is a fee on transfer ERC20 as deposit, withdraw atomic call sequence is allowed with pre-fee token quantity being accounted as deposit amount. Suppose F is a fee on transfer token and the pool with F as deposit...

Attacker could make deposits of 1 wei in the yield contract to prevent excess rewards from being withdrawn

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. Proof of Concept If an attacker makes many deposits of 1 wei the staking pool creator will have to make the withdraws himself to remove the unclaimed reward tokens. This can mean the pool creator will...

SpeedBumpPriceGate.sol and FixedPricePassThruGate.sol should check whether gate.beneficiary is address(0)

Lines of code Vulnerability details Impact In SpeedBumpPriceGate\addGate and FixedPricePassThruGate\addGate, it doesn’t check whether gate.beneficiary is address0. Therefore, when doing passThruGate. ETH will be sent to address0. These ETH can never be taken back. Proof of Concept beneficiary can...

Fund loss in passThruGate() of FixedPricePassThruGate becasue only some portion of user payed amount has been used and the rest of it don't returned to user

Lines of code Vulnerability details Impact If user pay extra ether for minting NFT, then those extra ethers will be locked in FixedPricePassThruGate forever. because passThruGate of FixedPricePassThruGate transfer only NFT cost to gate.beneficiary and don't return extra amount in msg.value to buy...

Multiple vestings for the same user will fail

Lines of code Vulnerability details Impact Loss of funds from multiple vestings for a single user Proof of Concept In MerkleVesting and MerkleResistor vestings are distributed using merkle trees. Creators of the vesting submit the Merkle root of the tree and deposit the funds to be distributed. A...

If treeAdder call addMerkleTree() of MerkleIdentity with wrong values for eligibilityIndex or priceIndex (uninitialized) attacker can steal NFTs

Lines of code Vulnerability details Impact If treeAdder call addMerkleTree of MerkleIdentity with wrong values for eligibilityIndex or priceIndex uninitialized gates index attacker can create those gate indexes in priceGateAddress or eligibilityAddress they are permission less with his own specif...

Creator of the contract could front run tax per capita to 100% of Yield rewards

Lines of code Vulnerability details Impact Contract creator could steal all rewards using frontrunning Proof of Concept When a yield pool is created pool tax is set equal to global tax and funds are sent into the contract to pay for rewards. The contract creator could set tax to 100% in a...

Pools and trees may be underfunded for fee-on-transfer tokens

Lines of code Vulnerability details Pools, vesting trees, and airdrop trees may all be created with fee-on-transfer tokens. When each of these entities is funded by a transfer in, their internal accounting assumes they receive the full amount transferred. However, they may actually receive fewer...

Tokens having more than 18 decimals are not supported

Originally submitted by warden pauliax in 173, duplicate of 49. Tokens having more than 18 decimals are not supported, the calculation will revert here: function decimalMultiplieraddress token internal view returns uint256 return 1018 - IERC20Fulltoken.decimals; --- The text was updated...

Transfers Will Revert On Non-Standard ERC20 Token Contracts

Lines of code Vulnerability details Impact There are several contracts which do not utilise OpenZeppelin's SafeERC20 library when performing token transfers. The FactoryDAO suite of smart contracts intends to support transfers by all tokens, including non-standard tokens such as USDT which does n...

Users may send more ETH in the passThruGate()

Lines of code Vulnerability details Impact When a user send ETH via the withdraw of MerkleIdentity, It will call IPriceGatetree.priceGateAddress.passThruGatevalue: msg.valuetree.priceIndex, msg.sender;. passThruGate checks whether msg.value = gate.ethCost holds. If a user accidentally sends more...

Pool Creators Can Reject Taxes From Being Withdrawn

Lines of code Vulnerability details Impact Upon pool creation, stakers can opt to earn rewards by staking their deposit tokens. When these deposit tokens are withdrawn, reward tokens are sent to the owner of the receipt along with their deposit tokens. A tax is charged on any rewards accrued by t...

The Contract Should approve(0) first

Originally submitted by warden defsec in 198, duplicate of 178 related to the use of safeApprove. This is upgraded from a QA report to standalone issue because it correctly described the revert when trying to call safeApprove on non-zero allowance. QA report that only describe safeApprove as...

COMP Distributions Can Be Manipulated And Duplicated Across Any Number Of Accounts

Lines of code Vulnerability details Impact The updateCompSupplyIndex and distributeSupplierComp functions are used by Compound to track distributions owed to users for supplying funds to the protocol. Bunker protocol is a fork of compound with NFT integration, however, part of the original...

CEther.doTransferOut() May Revert Because .transfer() Uses A Fixed Amount Of Gas

Lines of code Vulnerability details Impact The .transfer function intends to transfer an ETH amount with a fixed amount of 2300 gas. This function is not equipped to handle changes in the underlying .send and .transfer functions which may supply different amounts of gas in the future. Additionall...

Deprecated safeApprove() function

Originally submitted by warden Dravee in 146, duplicate of 178 related to the use of safeApprove. This is upgraded from a QA report to standalone issue because it correctly described the revert when trying to call safeApprove on non-zero allowance. QA report that only describe safeApprove as...

Use of deprecated Chainlink function latestAnswer

Lines of code Vulnerability details Impact Use of deprecated Chainlink function latestAnswer According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0, causing an incorrect price feed to USDC Price...

Deprecated Chainlink oracle API

Lines of code Vulnerability details Impact Deprecated Chainlink oracle API. API might stop working. Prices could be outdated. Protocol might need to be redeployed or false prices might lead to users losing funds. Proof of Concept The contracts use Chainlink’s deprecated API latestAnswer. Such...

call() should be used instead of transfer() on an address payable

Lines of code Vulnerability details This is a classic Code4rena issue: instead of call , transfer is used to withdraw the ether 2021-04-meebits-findings2 Swap.sol implements potentially dangerous transfer 2021-10-tally-findings20 OpenLevV1Lib's and LPool's doTransferOut functions call native...

Use latestRoundData instead latestAnswer of Chainlink aggregators

Lines of code Vulnerability details Impact latestAnswer function is deprecated, which doesn’t return an error but returns 0, and It is not able to check if the price is stale. Proof of Concept In the getUnderlyingPrice function of PriceOracleImplementation.sol, it uses...

CNft.sol - revert inside safeTransferFrom will break composability & standard behaviour

Lines of code Vulnerability details The function safeTransferFrom is a standard interface in ERC1155, and its expected to succeed if all the parametes are valid, and revert on error, which is not the case here so its a deviation. Refer to the EIP-1155 safeTransferFrom rules: MUST revert if to is...