MiniPool.Count state is not fully cleaned up

Lines of code Vulnerability details Impact MiniCount state is not fully cleaned up Proof of Concept According to the PR that aims to address M-19 code-423n4/2022-12-gogopool-findings235 We removed minipool count entirely, in favor of the new AVAXValidating variable that tracks the amount of AVAX...

amountAvailableForStaking() not fully utilized with compoundedAvaxNodeOpAmt easily forfeited

Lines of code Vulnerability details Impact The mitigated step is implemented at the expense of economic loss to both the node operators and the liquid stakers if compoundedAvaxNodeOpAmt ggAVAX.amountAvailableForStaking after all due to situations like liquid stakers have been actively calling...

Minipool whose multisig has been disabled cannot be reassigned a valid one

Lines of code Vulnerability details Impact The likelihood of startRewardsCycle reverting due to division by zero is practically not going to happen. This is because disableAllMultisigs is only reasonably invoked when pauseEverything is called by the defender. At his point, startRewardsCycle will...

Mitigation Confirmed for Mitigation of H-06 Issue mitigated

C4 issue H-06: MinipoolManager: node operator can avoid being slashed Comments In the original implementation, there were a few scenarios where malicious node operators can avoid being slashed. Mitigation PR 41 This PR includes mitigation for various issues H-03, H-06, M-13. Just focusing on the...

StRSR: attacker can steal excess rsr that is returned after seizure

Lines of code Vulnerability details Note: This issue deals with excess rsr that was seized from stRSR but is returned again. The M-12 issue also deals with excess rsr. However M-12 deals with the fact that not all rsr is returned to stRSR, whereas this issue deals with the fact that an attacker c...

There is no way to retrieve the rewards from the MultisigManager and rewards are locked in the vault.

Lines of code Vulnerability details C4 issue M-21: Division by zero error can block RewardsPoolstartRewardCycle if all multisig wallet are disabled. Comments The protocol provides an external function startRewardsCycle so that anyone can start a new reward cycle if necessary. Before mitigation,...

Deficiency of slashed GGP amount should be made up from node operator's AVAX

Lines of code Vulnerability details Impact If staked GGP doesn't cover slash amount, slashing it all will not be fair to the liquid stakers. Slashing is rare, and that the current 14 day validation cycle which is typically 1/26 of the minimum amount of GGP staked is unlikely to bump into this...

Upgraded Q -> 2 from #525 [1676219014177]

Judge has assessed an item in Issue 525 as 2 risk. The relevant finding follows: Upgradeable contract is missing a gap50 storage variable to allow for new storage variables in later versions --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #250 [1676238307490]

Judge has assessed an item in Issue 250 as 2 risk. The relevant finding follows: In first place, currentIndex which is of type mappingaddress = uint256 is incremented before using it in line 117. This will cause the implementation to miss the zero index, and start from the second place index 1. T...

Upgraded Q -> 2 from #59 [1676219064442]

Judge has assessed an item in Issue 59 as 2 risk. The relevant finding follows: L-06 Upgradeable contract is missing a gap50 storage variable to allow for new storage variables in later versions --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #272 [1676238909313]

Judge has assessed an item in Issue 272 as 2 risk. The relevant finding follows: function removeTokenEnumerationaddress from, address to, uint256 id, uint256 amount internal if to == address0 if idTotalSupplyid == 0 && additionalConditionRemoveTokenFromAllTokensEnumerationid...

Upgraded Q -> 2 from #353 [1676219078358]

Judge has assessed an item in Issue 353 as 2 risk. The relevant finding follows: 06 UPGRADEABLE CONTRACT IS MISSING A GAP50 STORAGE VARIABLE TO ALLOW FOR NEW STORAGE VARIABLES IN LATER VERSIONS --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #362 [1676219107265]

Judge has assessed an item in Issue 362 as 2 risk. The relevant finding follows: N‑01 Upgradeable contract is missing a gap50 storage variable to allow for new storage variables in later versions --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #504 [1676216850158]

Judge has assessed an item in Issue 504 as 2 risk. The relevant finding follows: Incorrect signature check in the validatePaymasterUserOp function --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #308 [1676219092947]

Judge has assessed an item in Issue 308 as 2 risk. The relevant finding follows: 03 Upgradeable contract is missing a gap50 storage variable to allow for new storage variables in later versions --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #157 [1676219053268]

Judge has assessed an item in Issue 157 as 2 risk. The relevant finding follows: L-08 No Storage Gap for BaseSmartAccount and ModuleManager --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #293 [1676241639792]

Judge has assessed an item in Issue 293 as 2 risk. The relevant finding follows: 01 USER CAN POSSIBLY TRANSFER NO token0 OR token1 TO TimeswapV2Option CONTRACT IF CORRESPONDING token0 OR token1 IS A REBASING TOKEN When calling the following TimeswapV2Option.mint function, msg.sender uses the...

Upgraded Q -> 2 from #533 [1676218902616]

Judge has assessed an item in Issue 533 as 2 risk. The relevant finding follows: Upgradeable contract is missing a gap50 storage variable to allow for new storage variables in later versions --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #250 [1676238274782]

Judge has assessed an item in Issue 250 as 2 risk. The relevant finding follows: Then, the logic presumably tries to keep a lookup table between token id - index using the ownedTokensIndex variable which is of type mappinguint256 = uint256 . This is also wrong, since ERC1155 tokens can have...

Adding NFTS with AssociationType ORDERED or PRIMARY may cause overwriting

Risk rating Medium Risk Links to affected code Impact Subprotocol NFTs may be trapped in contract CidNFT forever. Proof of Concept When adding NFT to CidNFT with AssociationType ORDERED or PRIMARY, the cidData is written directly, without checking and handling the case that a previously added nft...

Upgraded Q -> 2 from #214 [1675930440482]

Judge has assessed an item in Issue 214 as 2 risk. The relevant finding follows: cenario 2 - Use node of node operator In this scenario the NodeOp registers for a duration longer then 14 days. The hacker will hijack the minipool after 14 days and earn rewards on behalf of the node operators node...

Upgraded Q -> 2 from #510 [1675932817801]

Judge has assessed an item in Issue 510 as 2 risk. The relevant finding follows: If the current state is Withdrawable, you can still call createMinipool This will result in: 1:recreateMinipool can be front-run by executing recordStakingEnd to get back the stake first, and then executing...

Upgraded Q -> 3 from #510 [1675932827359]

Judge has assessed an item in Issue 510 as 3 risk. The relevant finding follows: In red are the state transitions that can only be performed with special privileges recreateMinipool: The following transitions will be performed Withdrawable-PreLaunch Error-PreLaunch createMinipool: will perform th...

MultiStakingReward.sol assumes all RewardTokens are in 18 decimal places

Lines of code Vulnerability details Impact Calculation of accrued rewards will be affected. Proof of Concept The function accrueRewards is called anytime rewards needs to be accrued. The variable supplyTokens is the total supply of the reward token. deltaIndex is calculated by taking the accrued ...

reentrancy in MultiRewardStaking::claimRewards for tokens with transfer callbacks, like erc777

Lines of code Vulnerability details Impact An attacker can drain all the tokens from MultiRewardStaking Proof of Concept In claimtRewards important state changes are done after interactions with tokens: File: MultiRewardStaking.sol function claimRewardsaddress user, IERC20 memory rewardTokens...

claimRewards is not re-entrancy safe.

Lines of code Vulnerability details Impact In MultiRewardStaking the function claimRewards doesn’t have nonReentrant which makes it possible to re-enter the function. If one of the reward tokens in ERC-777 token, it is possible to re-enter and claim the reward again and again until the contract i...

Incorrect computation in MultiRewardStaking changeRewardSpeed() leads to loss of rewards

Lines of code Vulnerability details Impact The changeRewardSpeed function computes rewardsEndTimestamp incorrectly for the case block.timestamp block.timestamp ? prevEndTime : block.timestamp.safeCastTo32, rewardsPerSecond, remainder If the prevEndTime block.timestamp then it can be reduced to...

RewardTokens can be locked in MultiRewardStaking contract when the rewardsEndTimestamp of the rewardsTokens are different.

Lines of code Vulnerability details Impact To claim reward tokens from the MultiRewardStaking contract deployed, a user must call claimRewardsaddress user, IERC20 memory rewardsTokens. The rewardsTokens array is populated with getAllRewardsTokens which returns all the reward Tokens the...

Missed owner accrual in MultiRewardStaking _withdraw() leads to reward loss

Lines of code Vulnerability details Impact Function withdraw can be called from an approved caller to withdraw owner funds. The function accrues rewards for caller and receiver but misses the accrual for owner. If, for example, the owner didn't accrue any reward from the beginning of time and all...

Re-entrancy in MultiRewardStaking.claimRewards

Lines of code Vulnerability details Impact If an ERC-777 token is used as reward token for any Staking contract in the system, that reward token can be completely drained from the Staking contract. Proof of Concept Re-entrancy can be done in the MultiRewardStaking.claimRewards function because of...

Risk of reentrancy attacks in the claimRewards function

Lines of code Vulnerability details Impact The claimRewards function in the MultiRewardStaking contract is used by users to claim token rewards, but because the function does not contain a nonReentrant modifier and does not implement the CEI standard check-effect-interact it can be subject to...

BeefyAdapter._protocolWithdraw() can revert for some boosters

Lines of code Vulnerability details When withdrawing from an adapter, the function does an internal call to protocolWithdraw 210: function withdraw 211: address caller, 212: address receiver, 213: address owner, 214: uint256 assets, 215: uint256 shares 216: internal virtual override 217: if calle...

MultiRewardStaking._accrueRewards can lead to loss of rewards for lower decimal tokens

Lines of code Vulnerability details Rewards accrual is computed in the following way: 406: deltaIndex = accrued.mulDivuint25610decimals, supplyTokens, Math.Rounding.Down.safeCastTo224; This can lead to truncation for low decimal tokens: Consider an instance with DAI as the asset, with a total of...

First depositor can break minting of shares

Lines of code Vulnerability details Vulnerability details The calculation of exchange rate for shares in Popcorn Vault is done by dividing the total supply of shares by the totalAssets of the vault. The first depositor can mint a very small number of shares, then donate to the vault to manipulate...

Out of gas for view function

Lines of code Vulnerability details Impact View function return's array of IERC20. There is no limit for this array Proof of Concept In one time owner of contract can add too much token's and it would take more and more gas to return this transation. Allocation in memory is not so cheap. After 20...

ERC4626 vault shares can be maliciously inflated

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. The price of ERC4626 vault shares can be maliciously inflated during the first deposit, leading to the loss of assets for next depositors Proof of Concept Provide direct links to all referenced code in...

Changing reward speed calculates wrong rewardsEndTimestamp

Lines of code Vulnerability details Impact In MultiRewardStaking.changeRewardSpeed the new rewardsEndTimetamp is calculated based on the current balance of reward tokens in the contract. However, a fraction of this balance might already be accrued and accounted as reward, but just has not been...

MultiRewardStaking.addRewardToken can eventually break the contract

Lines of code Vulnerability details When adding a reward token, the token address is added to rewardTokens. 263: rewardTokens.pushrewardToken; If rewardTokens is large enough, accrueRewards will revert with an out-of-gas error, as it loops through rewardsToken 373: for uint8 i; i 0...

managementFee is unfair and can be used to steal stakers deposits

Lines of code Vulnerability details Description managementFee is a fee that is taken on TVL and calculated per year: File: Vault.sol 429: function accruedManagementFee public view returns uint256 430: uint256 managementFee = fees.management; 431: return 432: managementFee 0 433: ?...

MultiRewardStaking does not work with fee-on-transfer tokens

Lines of code Vulnerability details The way MultiRewardStaking handles deposits does not work with fee-on-transfer tokens: deposit uses convertToShares to compute the number of shares to mint, which returns assets - ie a 1:1 equivalence between shares and the number of asset tokens. 98: function...

First depositor for the Vault can be front-run and have part of their deposit stolen

Lines of code Vulnerability details Description The first deposit with a totalSupply of zero shares will mint shares equal to the deposited amount. File: src/vault/Vault.sol 298: supply == 0 299: ? assets 300: : assets.mulDivsupply, totalAssets, Math.Rounding.Down; Link to Code File:...

Missing owner check in function addTemplate in DeploymentController

Lines of code Vulnerability details Impact Attacker can add malicous Vaults/Adatpors/Strategies template to TemplateRegistry. Attack can frontrun operator's transaction with the same templateCategory and templateId, but with a malicious Vault/Adatpor/Strategy template. If the operator does not...

ABI mismatch when calling initialize function

Lines of code Vulnerability details Impact This is the initialize function of contract Vault. It has 5 parameters. function initialize IERC20 asset, IERC4626 adapter, VaultFees calldata fees, address feeRecipient, address owner external initializer But when calling this function in VaultControlle...

Does not support non-18 decimals token

Lines of code Vulnerability details Impact Hardcode 1e18 for calculating fees and rewards, can make the template protocoal non flexiable for the calculating...

AdminProxy should do some extra security checks

Lines of code Vulnerability details Impact AdminProxy is the hot spot for all low-level calls, therefore it should do some extra security checks that are currently not in place. By design a Solidity low level call to a zero address or an EOA non contract address will return success true. The only...

MultiRewardEscrow.claimRewards() can break for rebasing tokens

Lines of code Vulnerability details Rebasing tokens make balanceOf modifications arbitrarily e.g: Aave share tokens. If such token is used in an escrow, the balance could become insufficient at the time of claiming rewards, making it impossible to claim rewards for that escrow. Impact Medium Proo...

Inflation attacks with virtual shares and assets

Lines of code Vulnerability details Impact When the BaseAdapter is empty. Someone can frontrun a user to steal his funds by an inflation attack. Senario Lets say Alice wants to deposit 1 token with decimal 18, so 1e18 units to the vault calling deposit. This is how the attack would unfold. The...

AdapterBase FEE_RECIPIENT is not implemented or updateable

Lines of code Vulnerability details Impact The FEERECIPIENT state variable in the AdapterBase.sol has an example address, TODO comment, and there is no way to update the value after deployment. This address is used to mint fees from the adapter when Adapter.harvest is called via the takeFees...

Any user can drain the entire reward fund in MultiRewardStaking due to incorrect calculation of supplierDelta

Lines of code Vulnerability details Impact Reward deltaIndex in accrueRewards is multiplied by 10decimals but eventually divided by rewards.ONE which is equal to 10IERC20MetadataaddressrewardToken.decimals in accrueUser. If the number of decimals in MultiRewardEscrow share token differs from the...

Wrong first parameter for _calcRewardsEnd when changing reward speed

Lines of code Vulnerability details Impact The function calcRewardsEnd is called with the previousEndTime as first parameter in MultiRewardStaking.changeRewardSpeed, which leads to wrong calculation of the new rewardsEndTimestamp, causing it to be later than it should be. This will lead to more...