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

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

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

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

Re-entrancy attack on the main functions

Lines of code Vulnerability details Impact A malicious token, or one that implemented transfer hooks, could re-enter the public calling function such as withdraw before proper internal accounting was completed. Because the earned reward function looks up the pool.totalDepositsWei and...

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

PermissionlessBasicPoolFactory\addPool() doesn’t check whether pool.excessBeneficiary is address(0)

Lines of code Vulnerability details Impact In PermissionlessBasicPoolFactory\addPool, it doesn’t check whether pool.excessBeneficiary is address0. Therefore, when doing withdrawExcessRewards. IERC20pool.rewardTokensi.transferpool.excessBeneficiary, rewards always revert. Proof of Concept...

SpeedBumpPriceGate.sol has high likelyhood of overcharging buyer during decay phase

Lines of code Vulnerability details Impact Contract overcharges user Proof of Concept L79 passes through the entire ether balance sent. Since price decays each block it is likely that the transaction won't be processed during the same block that it was submitted. In situations like this the price...

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

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

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

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

In withdraw() of MerkleIdentity if user set wrong value for merkleIndex, then treeAdder can perform front-running and steal user funds

Lines of code Vulnerability details Impact If user call withdraw of addMerkleTree with uncivilized merkleIndex, then it's possible for treeAdder to perform front-running attack and steal his funds. Proof of Concept This is code of withdraw in addMerkleTree: function withdrawuint merkleIndex, uint...

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

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

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

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

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

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

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

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

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

Beneficiary of SpeedBumpPriceGate can close the gate and hike the price for free.

Lines of code Vulnerability details Impact SpeedBumpPriceGate.sol is callable by anyone so the beneficiary can hike the price by calling it with his own ETH which will be returned to him or by making a flash loan to raise the price high enough that the gate is effectively closed indefinetely...

ERC20 transfers does not work on non-standard compliant tokens like USDT

Lines of code Vulnerability details Impact PermissionlessBasicPoolFactory.sol Consider this function: function deposituint poolId, uint amount external Pool storage pool = poolspoolId; requirepool.id == poolId, 'Uninitialized pool'; requireblock.timestamp pool.startTime, 'Cannot deposit before po...

FixedPricePassThruGate locks excess ETH payments

Lines of code Vulnerability details The FixedPricePassThruGate accepts ETH amounts greater than or equal to the calculated price, but only forwards an amount exactly equal to the calculated price to the configured beneficiary address. Excess ETH sent through the gate will be permanently locked in...

Re-Entrancy Attack in PermissionlessBasicPoolFactory

Lines of code Vulnerability details Impact Since, the state change of pool.rewardFunding is taking place after the ERC20.transferFrom function, and since there is no whitelist of ERC20 tokens, therefore, it is very much possible that a malicious ERC20 contract can initiate a re-entrancy attack fr...

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

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

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

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

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

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

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

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

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

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

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

Users will pay more than required for NFT Minting

Lines of code Vulnerability details Impact NFTs should be sold for their best price without descending into a gas race: Due to SpeedBumpPriceGate.sol function passThruGate code users will pay more than required for NFT Minting. User will pay msg.value and not the NFT "price". It is very likely th...

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

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

Using deprecated Chainlink function latestAnswer() in PriceOracleImplementation.sol

Lines of code Vulnerability details Impact In the function getUnderlyingPrice, Chainlink's latestAnswer is used and this function is deprecated and should not be used as this function returns 0 when there is no data rather than giving out error. Precedent of this issue can be found in...

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

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

Admin can manipulate nft prices by _setNftPriceOracle

Lines of code Vulnerability details impact setNftPriceOracle is an admin-only function which can set a new nftOracle. But there is no restriction in this function. An admin can set any oracle unconditionally. It could be used to manipulate nft prices in liquidateCalculateSeizeNfts. Proof of Conce...

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

Loss of NFTs Via Malicious Oracle Injection

Lines of code Vulnerability details Issue: The admin account can arbitrarily update the oracle at any time. If the oracle is changed to a malicious or vulnerable oracle, the price may be manipulated to allow illegitimate siezure of the assets. Consequences: Any user-deposited NFT with outstanding...

Oracle Prices Could Be Stale Or Manipulated due to latestAnswer() call

Lines of code Vulnerability details Impact In 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 in the...

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