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

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

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

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

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

Lines of code Vulnerability details Impact If a user calls depositTokens with wrong treeIndex value by mistake, attacker can perform front-running attack and steal user funds before user transaction reach the contract, attacker will create multiple trees in MerkleDropFactory until he reach tree...

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

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

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

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

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

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

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

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

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

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

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

Unchecked return value of transferFrom() leading to zero cost RewardFunding

Lines of code Vulnerability details Impact Due to unchecked return value of transferFrom in https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.solL144 , it is possible to fund a pool during creation without transferring tokens to use as rewardToken...

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

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

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

Not checking returned bool by transfer can lead to loss of funds

Lines of code Vulnerability details Impact Loss of funds Proof of Concept The ERC20 interface ensures a token transfer will return false on failure. In merkleVesting there is no requirement for this to be true. The contract doesn't ensure all the funds to cover the MerkleTree are present since it...

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Loss of Funds 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 undercollateralized borrowing. Consequences: Total loss of protocol funds for an attacker...

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

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

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

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

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

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

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

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