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

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

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

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

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

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

using tx.origin does not guarantee EOA (3)

Lines of code Vulnerability details Impact This condition does not guarantee that msg.sender is EOA. The best is to use !isContractmsg.sender. Having tx.origin == msg.sender is possible to bypass from another smartcontract which means attacker could have access to whitelist-protected functions...

A malicious actor can cause DoS with Block Gas Limit and destroy the sale flow also having advantage of buying cheaper price

Lines of code Vulnerability details Impact A malicious actor can cause DoS with Block Gas Limit and mint NFT's on cheaper price as the price drop is in action or destroy the sale flow. Proof of Concept Each block has an upper bound on the amount of gas that can be spent, and thus the amount...

reentryattack with owner checks and effecst pattern not implemented

Lines of code Vulnerability details Impact reentryattack can happen because checks and effects pattern not implemented Proof of Concept a owner can be millousius and take advantage of effects after a call Recommended Mitigation Steps do the checks and effects pattern --- The text was updated...

Missing validations in setter and setPhaseTimes() functions.

Lines of code Vulnerability details Impact Time variable validations daStartTime, mintlistStartTime, publicStartTime, claimsStartTime, selfRefundsStartTime are only done in setPhaseTimes and not even all validations. These validations should be performed: daStartTime block.timestamp...

Limit not enforced on teamSummon function

Lines of code Vulnerability details Impact The docs say there is a cap on how many tokens the project team can mint, however there are no checks or tracking implemented in the teamSummon function to enforce that limit. An admin calling that function could accidentally or maliciously exceed the...

Outdated Interface Implementation

Lines of code Vulnerability details Impact The Chainlink aggregator interface used by the project is outdated and does not permit the project to properly sanitize the price values it receives. Recommended Mitigation Steps The interface is to be updated according to the official Chainlink...

Dutch Auction fails when maxDaSupply is not reached

Lines of code Vulnerability details Impact Dutch Auction fails when maxDaSupply is not reached Proof of Concept Dutch Auction should update finalPrice every time price decreases. However, currently fianlPrice is updated only when auction reaches full supply reserved for auction phase. 1. Alice bu...

VaultCore's withdrawETH uses transfer function, which can be unusable for some smart contracts

Lines of code Vulnerability details Impact When a user wishes to withdraw their collateral from a WETH vault, the funds are returned to the user with msg.sender.transfer. Using transfer is not recommended anymore, especially for critical operations like withdrawing collateral from a vault as it...

Missing 0 approval

Lines of code Vulnerability details Impact When changing the allowance value from an existing non-zero value, certain tokens e.g., USDT must first be approved by zero before approving the actual allowance. Otherwise the token will not work. Proof of Concept There are two instances of missing zero...

[WP-H1] A malicious early user/attacker can manipulate the vault's pricePerShare to take an unfair share of future users' deposits

Lines of code Vulnerability details This is a well-known attack vector for new contracts that utilize pricePerShare for accounting. / @notice Calculates the number of shares that should be minted or burnt when a user deposit or withdraw. @param tokens Amount of asset tokens @return Number of...

Unchecked oracle return value

Lines of code Vulnerability details Impact The return value bool success of oracle.get calls is ignored. This could lead to stale data or incorrect prices due to oracle issues. Proof of Concept NFTPairWithOracle.solL287 Change to bool success, uint256 rate = loanParams.oracle.getaddressthis,...

Borrower can update loan parameters and force lender into accepting near-unliquidatable loan

Lines of code Vulnerability details Impact While loan is in LOANREQUESTED phase, the updateLoanParameters function can be called by the borrower to adjust TokenLoanParams.ltvBPS to a value that is very unfavorable for a prospective lender. Usually, this results in the loan not being accepted by a...

Lender can immediately liquidate valued Collateral in NFTPairWithOracle contract

Lines of code Vulnerability details Impact A lender can liquidate a borrower's collateral immediately by calling updateLoanParams in the NFTPairWithOracle contract where the ltvBPS for the params struct is set to 0. This bypasses the checks to make sure that the terms are favourable to the...

Lender can adjust ltvBPS and instantly sieze collateral

Lines of code Vulnerability details Impact After the loan is accepted and is in the LOANOUTSTANDING phase, updateLoanParams allows the lender to update the TokenLoanParams.ltvBPS parameter. The require check allows for the parameters to change as long as they are favorable for the borrower, but...

Malicious lender can change price oracle for outstanding loan

Lines of code Vulnerability details The updateLoanParams function in NFTPairWithOracle.sol allows the lender to update parameters for an outstanding loan duration, valuation, annual interest, and collateralization ratio as long as they are the same or better for the borrower. These params are...

[WP-H8] Special ERC721 compatible implementation may allow an attacker to requestLoan without transferring in the NFT collateral

Lines of code Vulnerability details NFT is a fragmented standard, for certain non-standard ERC721 implementations, they may have built-in hooks that can be used to re-enter the contract. Just like ERC777 to ERC20. For example, if the collateral NFT got a pre-transfer hook to the receiver of the...

RewardsController Emission Manager Can Authorize Users to Claim on Behalf of the AaveV3YieldSource Contract and Siphon Yield

Lines of code Vulnerability details Impact The AaveV3YieldSource contract allows the manager or owner of the contract to claim rewards from Aave's rewards controller. However, there is an external dependency on this periphery Aave contract such that the emission manager of the RewardsController...

Manager or owner can send rewards to any address

Lines of code Vulnerability details Impact In the claimRewards function, manager or owner can send rewards to any address. function claimRewardsaddress to external onlyManagerOrOwner returns bool requireto != address0, "AaveV3YS/payee-not-zero-address"; address memory assets = new address; assets...

claimRewards() didnt follow the safe check effect pattern

Judge @GalloDaSballo has assessed the 2nd item in QA Report 230 as Medium risk. The relevant finding follows: … Impact a user can claim a reward by calling the claimRewards, however this function didnt follow the correct check effect pattern, where the zero address is set after making an external...

_amount requires to be updated to contract balance increase (2)

Lines of code Vulnerability details Impact Every time transferFrom or transfer function in ERC20 standard is called there is a possibility that underlying smart contract did not transfer the exact amount entered. It is required to find out contract balance increase/decrease after the transfer. Th...

Potential reentrance in claimRewards

Judge @GalloDaSballo has assessed the 1st item in QA Report 36 as Medium risk. The relevant finding follows: … POC IERC20tokensi.safeTransfermsg.sender, getting; rewardmsg.sendertokensi = 0; Considering there are exterTokens, it is possible that some token will provide reentry opportunities...

Shelter donations result in wrong internal balance if fee-on-transfer token is used

Judge @GalloDaSballo has assessed the 2nd Low-severity item in QA Report 66 as Medium risk. The relevant finding follows: … In case of a fee-on-transfer ERC20 token the savedTokens balance will not represent the actual balance of the contract: You can either disable the use of fee-on-transfer...

Reward lost

Judge @GalloDaSballo has assessed the 11th item in QA Report 26 as Medium risk. The relevant finding follows: … Contract: In notifyRewardAmount function, if Admin added a reward 100 once block.timestamp = periodFinish. Now if Admin decides to add 200 rewards calling this function at block.timesta...

ChainlinkOracleProvider can provide zero and stale prices

Lines of code Vulnerability details Impact As stale price is determined by time since last timestamp, the price that is most recent, but wasn't updated for more than 2 hours say there were no trades on the market will be rejected, which makes system functionality unavailable in such a case. This...

function lockFunds in TopUpActionLibrary can cause serious fund lose. fee and Capped bypass. It's not calling stakerVault.increaseActionLockedBalance when transfers stakes.

Lines of code Vulnerability details Impact In function TopUpActionLibrary.lockFunds when transfers stakes from payer it doesn't call stakerVault.increaseActionLockedBalance for that payer so stakerVault.actionLockedBalancespayer is not get updated for payer and...

Index mint and burn calls can be front run

Lines of code Vulnerability details Impact Both in the mint and burn cases all the user supplied / due to a user assets can be stolen by an attacker, who detects correspondingly asset transfer calls / Index token transfer call and front runs Index contract's mint / burn call with own address as a...

User funds stuck - buy works even after sale ends

Lines of code Vulnerability details Impact Buy function is not checking finalized value. Under specific scenario, Buy function will work even when sale has ended and finalize function has been called. If user buys any token at this moment then he wont be able to claim and will lose funds Proof of...

_mintSharesFor doesn’t check whether the pool is zero.

Lines of code Vulnerability details Impact In StakedCitadel.sol/mintSharesFor, it doesn’t check whether the pool is equal to zero. If pool == 0 and totalSupply != 0, mintSharesFor will revert. And In depositFor, it uses balance as pool. In consequence, when the CTDL balance of StakedCitadel is ze...

latestRoundData data insufficiently validated

Lines of code Vulnerability details Impact The data returned by the Chainlink latestRoundData function may be stale. There should be checks applied on the data received from Chainlink to validate that it is not stale. Proof of Concept The ChainlinkPriceOracle contract has these two lines , int...

JPEGLock lockups overwrite previous data

Lines of code Vulnerability details Impact If a user calls NFTVault.finalizePendingNFTValueETH a second time without first calling JPEGLock.unlock to recover their previous lockup, their balance will be overwritten leaving the previous lockup balance unrecoverable. Proof of Concept POC by adding...

Use of deprecated oracle API in _normalizeAggregatorAnswer

Lines of code Vulnerability details Use of deprecated oracle API in normalizeAggregatorAnswer Likelihood low, impact high. The Chainlink latestAnswer function included in IAggregatorV3Interface and called in NFTVaultnormalizeAggregatorAnswer is considered deprecated and no longer included in the...

!_account.isContract() can be bypassed

Lines of code Vulnerability details Impact A contract in construction can bypass isContract to call deposit and withdraw functions in vaults/yVault/yVault.sol, farming/LPFarming.sol, and farming/yVaultLPFarming.sol. Also, Block contracts may cause DoS if someone uses multisig contracts as a calle...

Use of deprecated Chainlink function latestAnswer

Lines of code Vulnerability details function normalizeAggregatorAnswerIAggregatorV3Interface aggregator internal view returns uint256 int256 answer = aggregator.latestAnswer; uint8 decimals = aggregator.decimals; requireanswer 0, "invalidoracleanswer"; //converts the answer to have 18 decimals...

Wrong lockFor can lost tokens

Lines of code Vulnerability details Impact If the owner incorrectly uses the lockFor method and calls it twice with the same nftIndex, it will overwrite the record and the deposited jpeg can never be retrieved. Proof of Concept Owner call lockForAlice,0,100; Owner call lockForBob,0,100; 100 token...

Locked amount incorrectly updated in JPEGLock.sol

Lines of code Vulnerability details Impact The lockAmount gets overwritten each time the lockFor function is called in JPEGLock.sol. If the function is called multiple times, the parameter will be updated wrongly. Proof of Concept The lockAmount parameter gets assigned with the amount locked each...

yVault: First depositor can break minting of shares

Lines of code Vulnerability details Details The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”. Proof of Concept Attacker deposits 1 wei to mint 1...

currentLoanOwner can manipulate loanInfo when any lenders try to buyout

Lines of code Vulnerability details Impact If an attacker already calls lend to lend to a loan, the attacker can manipulate loanInfo by reentrancy attack when any lenders try to buyout. The attacker can set bad values of lendInfo e.g. very long duration, and 0 interest rate that the lender who...

Lend and borrow tickets can be transferred to the NFTLoanFacilitator

Lines of code Vulnerability details Impact Both lend and borrow tickets can be intentionally or accidentally transferred to the NFTLoanFacilitator contract. Since the NFTLoanFacilitator has no mechanism for rescuing these tokens or preventing their transfer, borrowers may be unable to repay,...

Loan can not be repaid when the BorrowTicket's owner is a contract with no onERC721Received method

Lines of code Vulnerability details In repayAndCloseLoan, after the loan is repaid, the Collateral NFT will safeTransferFrom addressthis to the owner of the BorrowTicket NFT, which is the mintBorrowTicketTo used in createLoan, must implement the onERC721Received method for the safeTransferFrom to...

NFTLoanFacilitator: Unsupported fee-on-transfer tokens

Lines of code Vulnerability details Impact loanAssetContractAddress is specified by the user when the user creates a loan. When loanAssetContractAddress is fee-on-transfer tokens, in the lend function, the actual amount of tokens received by the contract will be less than the amount, so that the...

onlyBurner modifier missing

Lines of code Vulnerability details Impact onlyBurner modifier is missing in burn function. Since it is a public function anyone can burn FEI tokens. Proof of Concept Tools Used Manual analysis Recommended Mitigation Steps Add onlyBurner modifier to the burn function. --- The text was updated...

Can force borrower to pay huge interest

Lines of code Vulnerability details Impact The loan amount is used as a min loan amount. It can be matched as high as possible realistically up to the collateral NFT's worth to remain in profit and the borrower has to pay interest on the entire amount instead of just on the desired loan amount wh...

NonCustodialPSM can become insolvent as CPI index rises

Lines of code Vulnerability details Impact NonCustodialPSM mints and redeems VOLT to a chosen stablecoin at the current market rate minus a fixed fee. It is assumed that the difference to be covered with pcvDeposit funds. That assumption is similar to one used in FEI protocol, but there no rate...

Missing balance check before permitting

Lines of code Vulnerability details Impact there is no owner balance checking before permit amount to spender. So owner can permit more than its balance to spender. Maybe this issue is not very risky but checking owner balance before permitting is more safe Proof of Concept Provide direct links t...