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

Lower LVT is treated as less restrictive, while it's vice versa

Lines of code Vulnerability details Impact Lender can accept overly restrictive LTV the lowest possible at the moment, with high enough probability being able to seize the collateral after a short time. Lender can set ltvBPS to zero with and immediately liquidate with removeCollateral any loan no...

Owner or Managers can rug Aave rewards

Lines of code Vulnerability details Impact A malicious owner or manager can steal all Aave rewards that are meant for PoolTogether users Even if the user is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. Proof of Concept File:...

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

Frontrun attack to steal first depositor money

Lines of code Vulnerability details Impact Frontrun attack to steal first depositor money Proof of Concept The flow is as following: 1. The first depositor wants to deposit X. 2. We detect it and frontrun 2 operations: 1. We deposit 1 of the underlying to the system. In exchange we receive 1 shar...

Skim Accidental Transfer of Collateral

Lines of code Vulnerability details Issue: removeCollateral function does not perform any address checks if the loan status = LOANINITIAL. Consequences: If anyone sends an NFT to the contract by itself, for whatever reason, an attacker can snatch it with removeCollateral. Proof of Concept Someone...

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

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

Reentrancy in removeCollateral() / requestLoan()

Lines of code Vulnerability details Impact There is a potential reentrancy bug they may exist between requestLoan and removeCollateral that allows a user to have requested a loan while maintaining ownership of the collateral. This bug is present in both NFTPair and NFTPairWithOracle. The reentran...

Avoidance of Liquidation Via Malicious Oracle

Lines of code Vulnerability details Issue: Arbitrary oracles are permitted on construction of loans, and there is no check that the lender agrees to the used oracle. Consequences: A borrower who requests a loan with a malicious oracle can avoid legitimate liquidation. Proof of Concept Borrower...

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

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

Loss of Collateral Via Malicious Oracle

Lines of code Vulnerability details Issue: The oracle can be arbitrarily updated at any point in time by the lender. Consequences: A lender can inject a malicious oracle at any time and steal the collateral NFT at the cost of his loaned tokens. Proof of Concept Borrower requests loan with an hone...

Missing support for ERC20 with fee

Lines of code Vulnerability details Impact Contract AaveV3YieldSource allows depositing tokens via supplyTokenTo function. Amount of tokens to transfer is based on passed argument depositAmount and is missing support for tokens with built-in fees. One of the popular tokens that implements such a...

Borrower can frontrun lender's call to lend to change the price oracle

Lines of code Vulnerability details Impact In NFTPairWithOracle.lend, params.oracle is not checked. This allow a borrower to watch the mempool and front-run the lender's call and change oracle to avoid liquidation. Proof of Concept function lend address lender, uint256 tokenId, TokenLoanParams...

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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