_transferNFTs() functions doesn't reverts if the transfer item doesn't supports interface for both ERC721 and ERC1155

Lines of code Vulnerability details Impact The transferNFTs function use ERC165 to check if the itemnft supports ERC721 interface or ERC1155 interface and execute transfer accordingly. But if it doesn't supports either, it just exits the functionno revert. Proof of Concept in...

_sendTokenToBribesProcessor() doesn't check bribesProcessor's address. Could cause permanent loss of fund

Lines of code Vulnerability details Impact In sendTokenToBribesProcessor, it sends tokens to bribesProcessor. It seems to be ok because claimBribesFromHiddenHand will confirm that bribesProcessor is not address0. sweepRewardToken also triggers sendTokenToBribesProcessor. But it doesn’t check...

prepareWithdrawAll should not be external

Lines of code Vulnerability details Impact Despite commented as Internal Core Implementations, prepareWithdrawAll is external. There also lack guarantee that prepareWithdrawAll is called before withdrawAll. Proof of Concept /// ===== Internal Core Implementations ===== function prepareWithdrawAll...

Timelock has a rug vector bypass

Lines of code Vulnerability details Impact Having a bypass for the timelock defeats the purpose of a timelock. Anyone in the EMERGENCYROLE, or who is able to compromise the key is able to immediately make changes at the expense of users. Proof of Concept executeEmergency can take any action on...

Upgraded Q -> M from 72 [1655579867255]

Judge has assessed an item in Issue 72 as Medium risk. The relevant finding follows: Use SafeTransferLib for all ETH send With the exception of issueRefund, other transfer should allow forward all gas to finish the transaction. To prevent case of costing more than 23000 gas for transfer. Line1...

Harvest is vulnerable to sandwich attack.

Lines of code Vulnerability details Impact Function harvest does multiple swaps from auraBAL - BAL/ETH BPT - WETH - AURA using BalancerVault. But it doesn’t use minAmountsOut or have a check for mimimum return amount. It makes this function vulnerable to sandwich attack. An attacker which can be ...

_processExtraToken() in _sendBadgerToTree() will transfer BADGER again

Lines of code Vulnerability details Impact sendBadgerToTree sends amount BADGER to BADGERTREE. However processExtraToken in sendBadgerToTree also sends amount BADGER to the vault. it sends amount BADGER twice. It could result in revert if it doesn't have enough balance of BADGER. Proof of Concept...

Principal payout

Lines of code Vulnerability details Impact It's possible to treat unvested aura as bribes and an attacker may cause a withdraw of AURA from the strategy to the popint where the debt in AURA to users cannot be covered by the strategy. Proof of Concept Anyone can create a valuable token in which it...

Missing slippage protection for autocompounding auraBAL rewards into AURA

Lines of code Vulnerability details Impact Autocompounding auraBAL rewards into AURA requires multiple swaps auraBAL - BAL/ETH BPT - WETH - AURA within MyStrategy.harvest. The swaps are at risk of being front-run / sandwiched, resulting in a loss of funds. Since MEV is very prominent I think the...

Upgraded Q -> M from 143 [1655579880057]

Judge has assessed an item in Issue 143 as Medium risk. The relevant finding follows: L-04 safeTransferETH should perform simple ETH transfers and don’t forward 30k gas Link: Being a simple funds transfer, having a fallback of a WETH deposit, there should be no extra gas involved when potentially...

Upgraded Q -> M from 205 [1655579891083]

Judge has assessed an item in Issue 205 as Medium risk. The relevant finding follows: transfer and send methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract...

Upgraded Q -> M from 155 [1655579831823]

Judge has assessed an item in Issue 155 as Medium risk. The relevant finding follows: ETH send return value is ignored while is gas limited The use of send / call to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the...

Emergency withdrawals are broken

Lines of code Vulnerability details Impact Usually, in emergency situations, contracts will be paused by the owner to prevent further damage. To withdraw all funds, the MyStrategy.prepareWithdrawAll function has to be manually called right before BaseStrategy.withdrawToVault can be called see...

YearnCurveVaultOperator's depositETH can leave the remainder ETH funds frozen and unaccounted for, then utilized by another caller

Lines of code Vulnerability details depositETH effectively do not control the utilization of input token and can freeze WETH input funds in native ETH form on the contract balance when Yearn pool doesn't perform liquidity addition for any reason. Due to presence of the additional WETH - ETH step,...

Upgraded Q -> M from 153 [1655579808802]

Judge has assessed an item in Issue 153 as Medium risk. The relevant finding follows: L03 Callvalue:amt"" should be used instead of send In ForgottenRunesWarriorsGuild.withdrawAll. transfer and send should be avoided because they take a hard dependency on gas costs by forwarding a fixed amount of...

_harvest() performs market swaps without slippage control and is a subject to sandwich attacks

Lines of code Vulnerability details As a result trades happen at a manipulated price and end up receiving fewer tokens than current market price dictates. Placing severity to medium as impact here is a partial fund loss conditional only on big enough asset amount to be swapped: sandwich attacks a...

Withdrawing all funds at once to vault can be DoS attacked by frontrunning and locking dust

Lines of code Vulnerability details Impact All funds can be migrated withdrawn at once to the caller vault by using the BaseStrategy.withdrawToVault function which internally calls MyStrategy.withdrawAll. The latter function has the following check in place: MyStrategy.solL184-L187 require...

removeVault operator functions can freeze the funds invested there

Lines of code Vulnerability details Vault removal methods don't check if there are any funds still invested with the vault being removed. In the same time after vault was removed withdraws from it will not be available as non-zero checks in the corresponding functions will fail. Setting severity ...

Should also check balanceOfRewards in _withdrawAll()

Lines of code Vulnerability details Impact withdrawAll should check that all of your positions are unwinded. It does check balanceOfPool and LOCKER.balanceOf. Then withdrawToVault in BaseStrategy.sol can transfer all want to the vault. But it doesn’t check reward tokens. want could also be one of...

Attacker can steal money from the initializer function

Lines of code Vulnerability details Impact Your Using old openzeplin Verision which the initializer function is vulnerable to and an attacker can call it twice after it’s been initliazed and since it approves max tokens to those contract. Which then after it’s first initialization the attacker...

Vault can never fully be emptied

Lines of code Vulnerability details Impact Vault cannot be fully emptied Proof of Concept Whenever rewards are earned they are automatically locked into the the Aura Locker. Since that reward will then earn more rewards while locked, there will be more rewards to be collected when that lock is...

Any User May Transfer Rewards From AuraLocker To MyStrategy These Rewards Are Unaccounted For and Stuck

Lines of code Vulnerability details Impact Harvesting the rewards from AuraLocker incorrectly assumes that rewards are only transferred during harvest when LOCKER.getRewardsaddressthis is called. However it is possible for anyone to call AuraLocker.getRewardsaddressMyStrategy and transfer the...

auraBAL can be stuck into the Strategy contract

Lines of code Vulnerability details Impact The internal harvest function defined is responsible to claim auraBAL from the aura locker and within the function it swaps them to auraBAL - BAL/ETH BPT - WETH - AURA, finally it locks AURA to the locker to increase the position. For claiming auraBAL it...

Wrong Deadline

Lines of code Vulnerability details the deadline is the timestamp after which the transaction will revert. the goal of this field is that the caller can set a deadline for the transaction so the transaction will not succeed in any arbitrary time in the future, and after this deadline, they can...

Upgraded Q -> M from 47 [1655579885825]

Judge has assessed an item in Issue 47 as Medium risk. The relevant finding follows: 4. Don't use payable.transfer/payable.send The use of payable.transfer is heavily frowned upon because it can lead to the locking of funds. The transfer call requires that the recipient is either an EOA account,...

Yearn Curve vault can be removed without withdrawing all deposited tokens. Causing these token to be locked forever unless an owner is added this vault back.

Lines of code Vulnerability details Impact Yearn Curve vault can be removed without withdrawing all deposited tokens. Causing these token to be locked forever unless an owner is added this vault back. Proof of Concept function removeVaultaddress vault external onlyOwner requirevaultsvault !=...

Upgraded Q -> M from 44 [1655579898351]

Judge has assessed an item in Issue 44 as Medium risk. The relevant finding follows: 1. Usage of legacy ETH transfer function Risk Low Impact Contract ForgottenRunesWarriors for withdrawing ETH to vault uses send function, which has a fixed gas stipend and can fail. The reason behind this is that...

Upgraded Q -> M from 135 [1655579793739]

Judge has assessed an item in Issue 135 as Medium risk. The relevant finding follows: L send is not recommended for sending native token Since the introduction of transfer, it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance...

Yield can be lost due to not specifying limit when transferring auraBAL to BAL/ETH BPT

Lines of code Vulnerability details Impact In harvest, when swapping auraBAL to BAL/ETH BPT the limit variable which specifies the minimum amount of tokens that are to be received when singleSwap.kind=GIVENIN is set to 0. This means that when the swap is made, the transaction can be frontrun and...

Fee-on-transfer support

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

Yield can be lost due to not specifying limit when transferring WETH to Aura

Lines of code Vulnerability details Impact In the harvest function, when the contract swaps WETH to Aura, limit is set to 0 allowing an attacker to front run the transaction and cause substantional loss of yield. swapSingleSwap singleSwap, FundManagement funds, uint256 limit, uint256 deadline...

_harvest rewards can be stolen because it doesn't implement any slippage bounds

Lines of code Vulnerability details Impact Harvested funds stolen Proof of Concept harvest does not implement any kind of minimum out when calling the 3 consecutive swaps L249, L263 and L275 to get from auraBal to Aura. An attacker could easily sandwich the least liquid pool and steal all the...

Override function but with different argument location

Lines of code Vulnerability details IYearnVault.sol.withdraw inherent IStakingVault.sol.withdraw but the parameters does not match --- The text was updated successfully, but these errors were encountered: All reactions...

Loss of yield can occur due to not specifying minAmountsOut when exiting BAL/ETH pool

Lines of code Vulnerability details Impact When exiting the BAL/ETH pool, due to not specifying anything for minAmountsOut an attacker can frontrun the transaction and cause a large change in price in the pool. This in turn leads to a large impermanent loss which is realised when the strategy bur...

Wrong Deadline

Lines of code Vulnerability details the deadline is the timestamp after which the transaction will revert. the goal of this field is that the caller can set a deadline for the transaction so the transaction will not succeed in any arbitrary time in the future, and after this deadline, they can...

Update initializer modifier to prevent reentrancy during initialization

Lines of code Vulnerability details Impact The solution uses: "OpenZeppelin/[email protected]". This dependency has a known high severity vulnerability: Which makes MyStrategy contract vulnerable contract MyStrategy is BaseStrategy, ReentrancyGuardUpgradeable ... function...

Upgraded Q -> M from 243 [1655572117784]

Judge has assessed an item in Issue 243 as Medium risk. The relevant finding follows: Title: Using SafeERC20 library in ForgottenRunesWarriorsMinter.sol There are some token which are not implementing current ERC20 standard example: USDT, OmiseGo and BNB. Using SafeERC20 library will be nice to...

User can bypass entryFee by sending arbitrary calldata to ParaSwap operator

Lines of code Vulnerability details Impact Any user is able to bypass the entryFee collection when using NestedFactory.create by passing in arbitrary calldata when using the ParaSwap router. High level, a user can pass in calldata to swap from a miniscule amount of input token to an ERC777 with...

Upgraded Q -> M from 270 [1655579826704]

Judge has assessed an item in Issue 270 as Medium risk. The relevant finding follows: Gas stipend for payable.send may be too low for contract wallets ETH withdrawals in both the minter and token contracts use payableaddress.send to transfer ether to the vault address. If the configured vault is ...

BADGER bribes can not be claimed

Lines of code Vulnerability details sendBadgerToTree will send BADGER twice and therefore fail. It is sending it to the BADGERTREE in sendBadgerToTree, and then continues to send the same amount to the vault in processExtraToken. Impact BADGER rewards cannot be claimed. The contract is trying to...

Swap routes are hardcoded for pools that do not exist yet.

Lines of code Vulnerability details Impact The route for swapping auraBAL to AURA is hardcoded and does not allow any flexibility. Proof of Concept The route for this swap is hardcoded to auraBAL - BAL/ETH BPT - WETH - AURA, with specific pool IDs. This seems to be done for the sake of simplicity...

Upgraded Q -> M from 119 [1655579861612]

Judge has assessed an item in Issue 119 as Medium risk. The relevant finding follows: L01: Use .call instead of .send Line References Description It is recommended to use call instead of send because the former fowards all remaining gas with the call, while the latter has a gas limit of 2300. Thi...

TODO: Hardcode claim.account = address(this)?

Lines of code Vulnerability details Impact Why you still has a TODO in the final code? TODO: Hardcode claim.account = addressthis? It is not implemented yet. claim.account may be any value, which may break the claiming process or let user steal fund that intended to be used in MyStrategy to their...

Beefy vault can be removed without withdrawing all deposited tokens. Causing these token to be locked forever unless an owner is added this vault back.

Lines of code Vulnerability details Impact Beefy vault can be removed without withdrawing all deposited tokens. Causing these token to be locked forever unless an owner is added this vault back. Proof of Concept function removeVaultaddress vault external onlyOwner requirevaultsvault != address0,...

Wrong initial value of exitFees and EntryFees

Lines of code Vulnerability details Impact L49/53/159/167 - The exitFees and entryFees variables can only have values ​​between 1 and 10000, which represent 0.01% and 100%, but since it is not set in the constructor, the default value is 0, a value that is incorrect. Recommended Mitigation Steps ...

Fee-on-transfer support

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

Operator may be removed without checking whether are there fund locked in that operator.

Lines of code Vulnerability details Impact Operator may be removed without checking whether are there fund locked in that operator. Locked fund may not be able to withdraw unless operator is being added back. Proof of Concept /// @inheritdoc INestedFactory function removeOperatorbytes32 operator...

If token == BADGER, the _handleRewardTransfer function does not work

Lines of code MyStrategy.solL405-L413 BaseStrategy.solL346-L353 Vault.solL396-L415 Vulnerability details Impact In MyStrategy.sol, when claiming bribes or sweeping reward tokens, the handleRewardTransfer function is called which calls sendToBadgerTree to send the amount of BADGER in the contract ...

_harvest has no slippage protection when swapping auraBAL for AURA

Lines of code Vulnerability details Impact Single swaps of harvest contains no slippage or deadline, which makes it vulnerable to sandwich attacks, MEV exploits and may lead to significant loss of yield. Proof of Concept When using BALANCERVAULT.swap here and here, there is no slippage protection...

Should it transfer underSpentAmount to _msgSender() instead of reserve?

Lines of code Vulnerability details Impact underSpentAmount is dust amount, if being transferred to reserve without any state modification. These tokens might be lost. Normally, dust amount must transfer back to only msgSender as a best practice. Proof of Concept unchecked uint256 underSpentAmoun...