[WP-M1] supplyTokenTo() may fail when Aave Pool address changed

Lines of code Vulnerability details function supplyTokenTouint256 depositAmount, address to external override nonReentrant uint256 shares = tokenToSharesdepositAmount; requireshares 0, "AaveV3YS/shares-gt-zero"; address underlyingAssetAddress = tokenAddress;...

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

Params.oracle can be changed to a fake one by front running NFTPairWithOracle's lend function

Lines of code Vulnerability details Impact As neither lend, nor updateLoanParams functions verify params.oracle, the lend call can be front run by a malicious borrower with changing params.oracle to a non-market one. The front run will be an updateLoanParams call where borrower introduces fake...

An attacker can make users' funds get "locked" in the contract (the owner can get them out and transfer them back to the users)

Lines of code Vulnerability details Impact If a user manages to be the first user to deposit into the contract, he will be minted shares and he can steal all the other users' deposits. Proof of Concept 1. The attacker deposits 1 token into the contract and 1 share is minted to him totalSupply and...

NFTPairWithOracle's _lend ignores accepted.oracle and allows to start loan with empty params.oracle

Lines of code Vulnerability details Impact As lend doesn't require params.oracle to be valid, while removeCollateral does, the loan initiation with an empty oracle can lead to ignoring collateral valuation. As the deals are OTC this can be seen as lender decision. However, lend ignores...

Yield source cannot deposit fee-on-transfer tokens

Lines of code Vulnerability details Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer or transferFrom is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert due to the contrac...

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

updateLoanParams can update to any malicious oracle address

Lines of code Vulnerability details Impact A lender attacker can modify the oracle address of tokenLoanParams to get collateral directly before expiration. Proof of Concept 1. First a lender attacker lends for a loan, the attacker can use updateLoanParams function and update to a malicious oracle...

Fee-on-transfer tokens will mess up the internal accounting of the contract

Lines of code Vulnerability details Impact If the underlying token is a fee-on-transfer token, the amount of tokens that will be transferred to the contract isn't equal to the amount the supplyTokenTo tries to supply to the aave protocol, so the function will revert because the contract won't hav...

Reentrancy attack in collateral.transferFrom that borrowers can trick lenders to lend but the lenders will never get the collateral

Lines of code Vulnerability details Impact A borrower attacker can use reentrancy attack to request a loan successfully and the collateral is still owned by the attacker. If a lender victim tries to call lend on the malicious loan which seems normal, the lender will lose money and never get the...

Outstanding loan Oracle can be arbitrary changed by a lender

Lines of code Vulnerability details Impact As updateLoanParams function do not verify params.oracle, a lender for an already outstanding loan can change params.oracle to a non-market one. For example, the lender can set oracle to a pre-cooked INFTOracle contract reporting 0 price of the asset and...

NFT oracle price request successful or not is not checked

Lines of code Vulnerability details Impact Loan can be destroyed because of no price reported by oracle. or any other reason that makes oracle "work as expected" in bad situations when the get function return success=false Proof of Concept According to the interface of INFTOracle, the first...

Protocol fees during origination are based on another fee rather than on the loan amount

Lines of code Vulnerability details Impact Protocol fee revenue will be much lower than expected. If the average NFT loaned on the platform is worth $900, openFeeShare will be $9, and protocolFeeShare will be only $0.90. There would have to be more than 50,000 such loans to cover the cost of this...

Possible to steal collateral during a reentrant collateral transfer

Lines of code Vulnerability details Impact In NFTPair.sol218 an ERC-721 transfer occurs. Anyone who gains execution during this transfer after the owner of the token is changed can steal the token transferred. Note that it will be applicable only if !skim. Since the exploit makes assumptions abou...

Blocking of Legitimate Liquidation

Lines of code Vulnerability details Issue: removeCollateral calculates whether liquidation is allowed via requirerate.mulloanParams.ltvBPS / BPS amount, "NFT is still valued";. An arbitrarily high ltvBPS will effectively bypass the oracle price and block liquidation. Consequences: A malicious...

Lender can unconditionally liquidate borrower by changing ltvBPS to 0

Lines of code Vulnerability details Impact In NFTPairWithOracle.updateLoanParams, a lender is allowed to decrease ltvBPS. If the lender set it to 0, he can call removeCollateral immediately to liquidate the borrower. Proof of Concept function updateLoanParamsuint256 tokenId, TokenLoanParams memor...

Calculation without check may result in tiny loss of user funds

Lines of code Vulnerability details Impact Calculation without the bigger than zero check may result in loss of user funds, albeit in tiny amounts as of now. Proof of Concept In this line of redeemToken shares to burn is calculated through tokenToShares method . As there is no check that checks i...

Borrower can frontrun lender's call to lend to increase ltvBPS and avoid liquidation

Lines of code Vulnerability details Impact In NFTPairWithOracle.lend, the loan ltvBPS can be higher than the lender's accepted ltvBPS. This allow a borrower to watch the mempool and front-run the lender's call and change ltvBPS to some very large value using updateLoanParams to avoid liquidation...

Inconsistent balance when supplying fee-on transfer tokens

Lines of code Vulnerability details Impact There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer or transferFrom. Proof of Concept AaveV3YieldSource.solL237 Tools Used Manu...

Initial supplyTokenTo() with _depositAmount=1 wei causes expensive share price

Lines of code Vulnerability details Impact The first depositor into the yield source is able to maliciously manipulate the share price by depositing the lowest possible amount 1 wei and then artificially blowing up the token balance. Following depositors will have to supply a larger amount of...

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

Reentrancy at _requestLoan allows requesting a loan without supplying collateral

Lines of code Vulnerability details requestLoan makes an external call to the collateral contract before updating the NFTPair contract state. Impact If the ERC721 collateral has a afterTokenTransfer hook, The NFTPair contract can be reentered, and a loan can be requested without the borrower...

Moral hazard of borrower calling liquidate() and potential Oracle manipulation

Lines of code Vulnerability details Impact In the InceptionVaultsCore contract, the liquidate and liquidatePartial function can be called anyone. This means that the borrower for a specific vaultId can call liquidate or liquidatePartial on his own vault. Furthermore, the project incentivizes...

Fund theft in redeemToken() because of rounding in division

Lines of code Vulnerability details Impact When user use redeemToken to get his tokens and burn his shares in FeildSource because of rounding in division user's share balance decrease is going to be lower than corresponding withdraw amount in value. for example if token's decimal was 1 and...

Unsupported fee-on-transfer tokens

Low-01 Unsupported fee-on-transfer tokens Impact When underlyingAssetAddress is fee-on-transfer tokens, in the supplyTokenTo function, the actual amount of tokens received by the contract will be less than the depositAmount, so that the subsequent pool.supply function will fail to execute. Proof ...

Early Depositor can DOS Deposits

Lines of code Vulnerability details Impact A malicious, but generous, early depositor can DOS all future deposits. This is accomplished by directly sending aTokens to the AaveV3YieldSource.sol contract after making their first deposit. The amount of aTokens sent to the contract will manipulate th...

User fund lose and DOS attack when totalSupply() is zero

Lines of code Vulnerability details Impact Hacker can do this two action: Perform a DOS attack and continuously deny users from supplying their tokens to YeildSource deposits less than a high amount will be rejected and attacker can control this amount, for example set it to 10K and every deposit...

User fund loss in supplyTokenTo() because of rounding

Lines of code Vulnerability details Impact When user use supplyTokenTo to deposit his tokens and get share in FeildSource because of rounding in division user gets lower amount of share. for example if token's decimal was 1 and totalSupply was 1000 and aToken.balanceOfFieldSource.address was 2100...

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

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

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

_depositAmount requires to be updated to contract balance increase

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

depositAmount requires to be updated to contract balance increase (6)

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

Improper Upper Bound Definition on the depositFeeBP

Judge @GalloDaSballo has assessed item C4-002 in QA Report 198 as Medium risk. The relevant finding follows: … Impact - LOW The add function does not have any upper or lower bounds. Values that are too large will lead to reversions in several critical functions. User funds will be locked forever...

withdrawFees() function shoud require to address to not be zero

Lines of code Vulnerability details Impact withdrawFees don't check that to address is not zero and send fee to the address without any check that confirms admin has set the address. bentoBox don't accept transferring to zero address, otherwise this could be high risk Proof of Concept As you can...

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

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

You should use safe casts here

Judge @GalloDaSballo has assessed the 6th item in QA Report 254 as Medium risk. The relevant finding follows: … You should use safe casts here: depositspidmsg.sender.amount += uint192amount; depositspidmsg.sender.amount -= uint192amount; amount : uint192amount Otherwise, if token amounts are...

Missing Re-entrancy Guard

Judge @GalloDaSballo has assessed item C4-009 in QA Report 198 as Medium risk. The relevant finding follows: … Impact - Non-Critical Consider using ReentrancyGuard to protect functions that have external calls and do not follow Checks Effects Interactions pattern. An example of a function that...

Potential Sandwich Attack: Arbitrage bots can front run reward tokens being sent to the liquidity mining contracts

Lines of code Vulnerability details Impact For the PARMiner and DemandMiner contracts, arbitrage bots could harvest significant portion of rewards by monitoring MEV, and front run any reward token either a.mimo or par being transferred to the liquidityMining contract i.e. call the deposit functio...

Variables that should be bounded

Judge @GalloDaSballo has assessed the 8th item in QA Report 263 as Medium risk. The relevant finding follows: … The variable MasterChef.sol:43: uint16 depositFeeBP; // Deposit fee in basis points is never bounded, and UInt16.MaxValue is 65535 --- The text was updated successfully, but these error...

Function recoverERC20 in StakingRewards allows an owner to transfer out any token except stakingToken

Judge @GalloDaSballo has assessed the 1st item in QA Report 254 as Medium risk. The relevant finding follows: … Function recoverERC20 in StakingRewards allows an owner to transfer out any token except stakingToken. I see 2 problems with this: 1. It should also forbid transferring of rewardsToken,...

Users' funds can become locked in the contract

Judge @GalloDaSballo has assessed the 2nd item in QA Report 163 as Medium risk. The relevant finding follows: … Users' funds can become locked in the contract In the withdraw function in ConvexStakingWrapper there's a call to MasterChef's withdraw funciton which uses the safeConcurTransfer functi...

Lack of require in redeemToken could produce token loses

Lines of code Vulnerability details Impact Lack of require in redeemToken could produce token loses. Proof of Concept In the method redeemToken the user set the expected redeemAmount, it will compute the expected shares to burn, and after it, it will transfer the amout according the redeemAmount...

Reentrency in claimRewards in ConcurRewardPool

Judge @GalloDaSballo has assessed the 1st item in QA Report 163 as Medium risk. The relevant finding follows: … Reentrency in claimRewards in ConcurRewardPool The function claimRewards is open to reenterncy, if the safeTransfer function of a token calls the claimRewards again the tokens can be...

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

When the owner uses the cloneInceptionVault function to create an InceptionVault, he can control the oracle address

Lines of code Vulnerability details Impact The owner can control the oracle address when creating an InceptionVault using the cloneInceptionVault function of the InceptionVaultFactory contract. A malicious owner can use create2 to deploy a normal oracle, and then use the oracle's address to creat...

amount requires to be updated to contract balance increase (5)

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

User fund stuck

Judge @GalloDaSballo has assessed the 3rd item in QA Report 26 as Medium risk. The relevant finding follows: … Contract: In withdraw function, Funds will stuck if user deposited a amount and then isDepositordepositor is set to false by Admin. Now user cannot withdraw the amount since onlyDeposito...

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

Cast to uint192 is unsafe

Judge @GalloDaSballo has assessed the 3rd item in QA Report 197 as Medium risk. The relevant finding follows: … Cast to uint192 is unsafe, user trying to deposit more than 2^192 would recevie less deposits balance. depositspidmsg.sender.amount += uint192amount; --- The text was updated...