Platform: Code4rena
Start Date: 15/12/2022
Pot Size: $128,000 USDC
Total HM: 28
Participants: 111
Period: 19 days
Judge: GalloDaSballo
Total Solo HM: 1
Id: 194
League: ETH
Rank: 8/111
Findings: 6
Award: $2,375.27
š Selected for report: 1
š Solo Findings: 0
š Selected for report: 0xdeadbeef0x
Also found by: 0Kage, 0xc0ffEE, 0xmint, AkshaySrivastav, Allarious, Ch_301, Franfran, HollaDieWaldfee, IllIllI, Jeiwan, Lirios, Manboy, adriro, ak1, bin2chen, caventa, chaduke, clems4ever, cozzetti, datapunk, imare, immeas, kaliberpoziomka8552, ladboy233, pauliax, peritoflores, rvierdiiev, sces60107, sk8erboy, stealthyz, unforgiven, wagmi, wallstreetvilkas, yixxas
4.9672 USDC - $4.97
When the state is MinipoolStatus.Withdrawable, the recreateMinipool() function expects that it can fetch the data from the old minipool, and re-create the pool using the existing funds, before changing the state to MinipoolStatus.Prelaunch. The same fetching is not done in createMinipool(), where it only clears all of the old data before changing the state.
Funds not withdrawn using withdrawMinipoolFunds() will be lost
If the pool already existed, the state transition allows the minipool data to be reset, and then overwritten with msg.value:
// File: contracts/contract/MinipoolManager.sol : MinipoolManager.createMinipool() #1 238 // Create or update a minipool record for nodeID 239 // If nodeID exists, only allow overwriting if node is finished or canceled 240 // (completed its validation period and all rewards paid and processing is complete) 241 int256 minipoolIndex = getIndexOf(nodeID); 242 if (minipoolIndex != -1) { 243 @> requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch); 244 resetMinipoolData(minipoolIndex); ... 261 setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt")), msg.value); 262: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);
The state transition is allowed from MinipoolStatus.Withdrawable to MinipoolStatus.Prelaunch:
// File: contracts/contract/MinipoolManager.sol : MinipoolManager.requireValidStateTransition() #2 163 } else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) { 164:@> isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
Code inspection
Use recreateMinipool() if the pool is found to already exist
#0 - 0xminty
2023-01-03T23:33:08Z
dupe of #805
#1 - c4-judge
2023-01-10T10:30:43Z
GalloDaSballo marked the issue as duplicate of #213
#2 - c4-judge
2023-02-03T12:33:01Z
GalloDaSballo changed the severity to 2 (Med Risk)
#3 - c4-judge
2023-02-03T19:26:10Z
GalloDaSballo changed the severity to 3 (High Risk)
#4 - c4-judge
2023-02-08T08:26:45Z
GalloDaSballo changed the severity to 2 (Med Risk)
#5 - c4-judge
2023-02-08T08:50:12Z
GalloDaSballo changed the severity to 3 (High Risk)
#6 - c4-judge
2023-02-08T20:23:04Z
GalloDaSballo marked the issue as satisfactory
#7 - c4-judge
2023-02-09T08:22:32Z
GalloDaSballo marked the issue as partial-50
#8 - GalloDaSballo
2023-02-09T08:22:41Z
Valid report but didn't show how this can be as an attack so awarding half
#9 - c4-judge
2023-02-09T08:53:06Z
GalloDaSballo changed the severity to QA (Quality Assurance)
#10 - Simon-Busch
2023-02-09T12:49:56Z
Changed severity back from QA to H as requested by @GalloDaSballo
š Selected for report: 0xdeadbeef0x
Also found by: 0xLad, 0xNazgul, 0xSmartContract, 0xbepresent, Arbor-Finance, Breeje, HE1M, IllIllI, Qeew, Rolezn, SEVEN, SamGMK, SmartSek, TomJ, WatchDogs, ak1, btk, ck, datapunk, dic0de, eierina, fs0c, hansfriese, koxuan, ladboy233, peanuts, rvierdiiev, sces60107, tonisives, unforgiven, yongskiws
14.9051 USDC - $14.91
An early depositor can break the future minting of shares by minting a very small number of shares, but donating a large amount of AVAX during the reward period, such that each wei of ggAVAX is worth a large amount.
If one wei of ggAVAX is worth, say, 1 Eth of AVAX, if a user calls deposit() with a value less than this amount, the amount the number of shares they get back round down to zero due to loss of precision and the call will revert, essentially bricking the contract for users with small amounts. If a user instead deposits an amount larger than 1 Eth, the amount over 1 Eth is given to all shareholders due to loss of precision.
An attacker can deposit 1 wei of AVAX, then transfer a large amount of AVAX to the TokenggAVAX contract via a selfdestruct(), then call syncRewards(). That function will convert the balance of the contract to lastRewardsAmt...:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.syncRewards() #1 99 @> uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_; 100 101 // Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`. 102 uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; 103 104:@> lastRewardsAmt = nextRewardsAmt.safeCastTo192();
...which is used in totalAssets() either in the next period, or proportionally in the current period, depending on how much time has elapsed...:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.totalAssets() #2 120 if (block.timestamp >= rewardsCycleEnd_) { 121 // no rewards or rewards are fully unlocked 122 // entire reward amount is available 123 @> return totalReleasedAssets_ + lastRewardsAmt_; 124 } 125 126 // rewards are not fully unlocked 127 // return unlocked rewards and stored total 128 @> uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_); 129 return totalReleasedAssets_ + unlockedRewards; 130: }
...and that total is used to determine the worth of every share during minting:
// File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol : ERC4626Upgradeable.convertToShares() #3 120 function convertToShares(uint256 assets) public view virtual returns (uint256) { 121 uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero. 122 123 @> return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets()); 124: }
Code inspection
Upon initialization, mint an initial amount of shares to an address that is not able to withdraw them
#0 - c4-judge
2023-01-08T13:11:30Z
GalloDaSballo marked the issue as duplicate of #209
#1 - c4-judge
2023-02-08T09:44:13Z
GalloDaSballo marked the issue as satisfactory
š Selected for report: unforgiven
Also found by: 0x73696d616f, 0xNazgul, Bnke0x0, Breeje, HollaDieWaldfee, IllIllI, Rolezn, __141345__, btk, ck, csanuragjain, fs0c, joestakey, kiki_dev, koxuan, rvierdiiev
40.8808 USDC - $40.88
Rewards can be deposited to the TokenggAVAX contract, but aren't used in share valuation until syncRewards() is called.
If a user sees that rewards have been deposited to ggAVAX and decides to withdraw, their withdrawal will not include the reward if it hasn't yet been synced, leading to the user missing out on rewards they should have gotten.
totalAssets() is used by ERC4626 to calculate the value of shares. Once a cycle has ended, it will include any previously-synced rewards (lastRewardsAmt), but will not include any rewards that have been deposited since then, e.g. due to an unexpected airdrop:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.totalAssets() #1 113 function totalAssets() public view override returns (uint256) { 114 // cache global vars 115 uint256 totalReleasedAssets_ = totalReleasedAssets; 116 uint192 lastRewardsAmt_ = lastRewardsAmt; 117 uint32 rewardsCycleEnd_ = rewardsCycleEnd; 118 uint32 lastSync_ = lastSync; 119 120 if (block.timestamp >= rewardsCycleEnd_) { 121 // no rewards or rewards are fully unlocked 122 // entire reward amount is available 123 @> return totalReleasedAssets_ + lastRewardsAmt_; 124: }
lastRewardsAmt only gets updated when things are synced:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.syncRewards() #2 99 @> uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_; 100 101 // Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`. 102 uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; 103 104: lastRewardsAmt = nextRewardsAmt.safeCastTo192();
Code inspection
In beforeWithdraw(), call syncRewards() if the contract balance is larger than the total stored amounts
#0 - c4-judge
2023-01-10T09:59:16Z
GalloDaSballo marked the issue as duplicate of #478
#1 - c4-judge
2023-02-08T20:11:44Z
GalloDaSballo marked the issue as satisfactory
š Selected for report: unforgiven
Also found by: 0x73696d616f, 0xNazgul, Bnke0x0, Breeje, HollaDieWaldfee, IllIllI, Rolezn, __141345__, btk, ck, csanuragjain, fs0c, joestakey, kiki_dev, koxuan, rvierdiiev
40.8808 USDC - $40.88
Reward cycles can start at any time after the end of the cycle, but always end at an exact multiple of the interval.
An attacker can time the syncing of unsynced rewards such that they only need a large AVAX loan for a single block, in order to capture most of the rewards.
The end of a cycle is always a multiple of the cycle length, even if that occurs only one block in the future:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.syncRewards() #1 99 uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_; 100 101 // Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`. 102 @> uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; 103 104 lastRewardsAmt = nextRewardsAmt.safeCastTo192(); 105 lastSync = timestamp; 106 rewardsCycleEnd = nextRewardsCycleEnd; 107 totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_; 108 emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt); 109: }
An attacker can sync one block before the end of the cycle and take out a very large AVAX loan, mint ggAVAX with it, wait one block, then redeem the shares and repay the loan. The value of the shares will have grown significantly since the totalAssets() will include the full value of unlockedRewards, since the next cycle will have ended after that one block:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.totalAssets() #2 126 // rewards are not fully unlocked 127 // return unlocked rewards and stored total 128 @> uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_); 129 return totalReleasedAssets_ + unlockedRewards; 130: }
As long as the rewards are larger than the one-block loan interest, it is worth it for the attack to take place.
Code inspection
Don't allow syncing if the time remaining of the cycle is less than some threshold
#0 - c4-judge
2023-01-10T17:47:35Z
GalloDaSballo marked the issue as duplicate of #478
#1 - c4-judge
2023-02-08T20:11:42Z
GalloDaSballo marked the issue as satisfactory
š Selected for report: 0xbepresent
Also found by: 0xdeadbeef0x, Breeje, Franfran, IllIllI, Matin, Rolezn, SmartSek, btk, ck, datapunk, fs0c, nadin, rvierdiiev, unforgiven
57.1995 USDC - $57.20
totalAssets() is used to calculate the value of shares when doing a withdrawal/redemption, but includes in its value, rewards that are not withdrawable.
The user won't be able to withdraw their full amount of assets, but if another user deposits the portion of totalAssets() that makes up the reward portion, the first user will be able to withdraw, and the second user will be locked in their position until the rewards are distributed during the next sync. This is possible any time there are only a couple of holders, or when there is a whale that has a very large position in relation to the other holders of ggAVAX.
totalAssets() includes "unlocked" rewards on top of the totalReleasedAssets:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.totalAssets() #1 126 // rewards are not fully unlocked 127 // return unlocked rewards and stored total 128 @> uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_); 129 return totalReleasedAssets_ + unlockedRewards; 130: }
But the beforeWithdraw() hook will revert if the full total is withdrawn unless it has been topped off by another user:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.beforeWithdraw() #2 241 function beforeWithdraw( 242 uint256 amount, 243 uint256 /* shares */ 244 ) internal override { 245 @> totalReleasedAssets -= amount; 246: }
The other user won't be able to withdraw until the rewards are synced at the end of the window:
// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.syncRewards() #3 106 rewardsCycleEnd = nextRewardsCycleEnd; 107:@> totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_;
Code inspection
Update the totalReleasedAssets with the unlocked portion of rewards, if someone does a withdrawal
#0 - c4-judge
2023-01-08T17:01:26Z
GalloDaSballo marked the issue as duplicate of #317
#1 - c4-judge
2023-02-08T10:06:40Z
GalloDaSballo marked the issue as satisfactory
š Selected for report: IllIllI
Also found by: 0xSmartContract, AkshaySrivastav, HollaDieWaldfee, RaymondFam, Rolezn, ast3ros, betweenETHlines, brgltd, chaduke, codeislight, cozzetti, lukris02, pauliax, sces60107
1538.3717 USDC - $1,538.37
| Issue | Instances | |
|---|---|---|
| [Lā01] | Inflation not locked for four years | 1 |
| [Lā02] | Contract will stop functioning in the year 2106 | 1 |
| [Lā03] | Lower-level initializations should come first | 1 |
| [Lā04] | Cycle end may be be too soon | 1 |
| [Lā05] | Incorrect percentage conversion | 1 |
| [Lā06] | Missing checks for value of msg.value | 1 |
| [Lā07] | Loss of precision | 2 |
| [Lā08] | Signatures vulnerable to malleability attacks | 1 |
| [Lā09] | require() should be used instead of assert() | 1 |
| [Lā10] | Open TODOs | 1 |
Total: 11 instances over 10 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Common code should be refactored | 1 |
| [Nā02] | String constants used in multiple places should be defined as constants | 1 |
| [Nā03] | Constants in comparisons should appear on the left side | 1 |
| [Nā04] | Inconsistent address separator in storage names | 1 |
| [Nā05] | Confusing function name | 1 |
| [Nā06] | Misplaced punctuation | 1 |
| [Nā07] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 1 |
| [Nā08] | Import declarations should import specific identifiers, rather than the whole file | 13 |
| [Nā09] | Missing initializer modifier on constructor | 1 |
| [Nā10] | The nonReentrant modifier should occur before all other modifiers | 2 |
| [Nā11] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 1 |
| [Nā12] | constants should be defined rather than using magic numbers | 2 |
| [Nā13] | Missing event and or timelock for critical parameter change | 1 |
| [Nā14] | Events that mark critical parameter changes should contain both the old and the new value | 2 |
| [Nā15] | Use a more recent version of solidity | 1 |
| [Nā16] | Use a more recent version of solidity | 1 |
| [Nā17] | Constant redefined elsewhere | 2 |
| [Nā18] | Lines are too long | 1 |
| [Nā19] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 2 |
| [Nā20] | Using >/>= without specifying an upper bound is unsafe | 2 |
| [Nā21] | Typos | 3 |
| [Nā22] | File is missing NatSpec | 3 |
| [Nā23] | NatSpec is incomplete | 27 |
| [Nā24] | Not using the named return variables anywhere in the function is confusing | 1 |
| [Nā25] | Contracts should have full test coverage | 1 |
| [Nā26] | Large or complicated code bases should implement fuzzing tests | 1 |
| [Nā27] | Function ordering does not follow the Solidity style guide | 15 |
| [Nā28] | Contract does not follow the Solidity style guide's suggested layout ordering | 9 |
Total: 98 instances over 28 issues
The litepaper says that there will be no inflation for four years, but there is no code enforcing this
There is 1 instance of this issue:
File: /contracts/contract/ProtocolDAO.sol 39 // GGP Inflation 40 setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days); 41: setUint(keccak256("ProtocolDAO.InflationIntervalRate"), 1000133680617113500); // 5% annual calculated on a daily interval - Calculate in js example: let dailyInflation = web3.utils.toBN((1 + 0.05) ** (1 / (365)) * 1e18);
limiting the timestamp to fit in a uint32 will cause the call below to start reverting in 2106
There is 1 instance of this issue:
File: /contracts/contract/tokens/TokenggAVAX.sol 89: uint32 timestamp = block.timestamp.safeCastTo32();
There may not be an issue now, but if ERC4626 changes to rely on some of the functions BaseUpgradeable provides, things will break
There is 1 instance of this issue:
File: /contracts/contract/tokens/TokenggAVAX.sol 72 function initialize(Storage storageAddress, ERC20 asset) public initializer { 73 __ERC4626Upgradeable_init(asset, "GoGoPool Liquid Staking Token", "ggAVAX"); 74: __BaseUpgradeable_init(storageAddress);
The first timestamp should be based on when things are first synced, so that an appropriate duration of the cycle occurs, rather than during deployment
There is 1 instance of this issue:
File: /contracts/contract/tokens/TokenggAVAX.sol 78: rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength;
0.2 ether should be 20%, not 2%. Other areas use 0.X as X0%
There is 1 instance of this issue:
File: /contracts/contract/MinipoolManager.sol 194: /// @param delegationFee Percentage delegation fee in units of ether (2% is 0.2 ether)
msg.valueOther functions in this contract such as recordStakingError() ensure that the correct value is passed, but recordStakingEnd() does not
There is 1 instance of this issue:
File: /contracts/contract/MinipoolManager.sol 385 function recordStakingEnd( 386 address nodeID, 387 uint256 endTime, 388 uint256 avaxTotalRewardAmt 389: ) external payable {
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There are 2 instances of this issue:
File: contracts/contract/RewardsPool.sol 60: return (block.timestamp - startTime) / dao.getInflationIntervalSeconds(); 128: return (block.timestamp - startTime) / dao.getRewardsCycleSeconds();
ecrecover() accepts as valid, two versions of signatures, meaning an attacker can use the same signature twice. Consider adding checks for signature malleability, or using OpenZeppelin's ECDSA library to perform the extra checks necessary in order to prevent this attack.
There is 1 instance of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 132 address recoveredAddress = ecrecover( 133 keccak256( 134 abi.encodePacked( 135 "\x19\x01", 136 DOMAIN_SEPARATOR(), 137 keccak256( 138 abi.encode( 139 keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"), 140 owner, 141 spender, 142 value, 143 nonces[owner]++, 144 deadline 145 ) 146 ) 147 ) 148 ), 149 v, 150 r, 151 s 152: );
require() should be used instead of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".
There is 1 instance of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 83: assert(msg.sender == address(asset));
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: contracts/contract/MinipoolManager.sol 412: // TODO Revisit this logic if we ever allow unequal matched funds
BaseAbstract performs similar operations, so the common code should be refactored to a function
There is 1 instance of this issue:
File: /contracts/contract/MultisigManager.sol 110: addr = getAddress(keccak256(abi.encodePacked("multisig.item", index, ".address")));
There is 1 instance of this issue:
File: /contracts/contract/MultisigManager.sol 110: addr = getAddress(keccak256(abi.encodePacked("multisig.item", index, ".address")));
Doing so will prevent typo bugs
There is 1 instance of this issue:
File: /contracts/contract/ClaimNodeOp.sol 92: if (ggpRewards == 0) {
Most addresses in storage names don't separate the prefix from the address with a period, but this one has one
There is 1 instance of this issue:
File: /contracts/contract/ProtocolDAO.sol 102 function getClaimingContractPct(string memory claimingContract) public view returns (uint256) { 103 return getUint(keccak256(abi.encodePacked("ProtocolDAO.ClaimingContractPct.", claimingContract))); 104 } 105 106 /// @notice Set the percentage a contract is owed for a rewards cycle 107 function setClaimingContractPct(string memory claimingContract, uint256 decimal) public onlyGuardian valueNotGreaterThanOne(decimal) { 108 setUint(keccak256(abi.encodePacked("ProtocolDAO.ClaimingContractPct.", claimingContract)), decimal); 109: }
Consider changing the name to stakeGGPAs or stakeGGPFor
There is 1 instance of this issue:
File: /contracts/contract/Staking.sol 328: function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
There's an extra comma - it looks like a find-and-replace error
There is 1 instance of this issue:
File: /contracts/contract/Vault.sol 154 // Update balances 155 tokenBalances[contractKey] = tokenBalances[contractKey] - amount; 156 // Get the toke ERC20 instance 157 ERC20 tokenContract = ERC20(tokenAddress); 158 // Withdraw to the withdrawal address, , safeTransfer will revert if it fails 159 tokenContract.safeTransfer(withdrawalAddress, amount); 160: }
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There is 1 instance of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 24: contract TokenggAVAX is Initializable, ERC4626Upgradeable, UUPSUpgradeable, BaseUpgradeable {
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation
There are 13 instances of this issue:
File: contracts/contract/Base.sol 4: import "./BaseAbstract.sol";
File: contracts/contract/BaseUpgradeable.sol 4: import "./BaseAbstract.sol";
File: contracts/contract/ClaimNodeOp.sol 4: import "./Base.sol";
File: contracts/contract/ClaimProtocolDAO.sol 4: import "./Base.sol";
File: contracts/contract/MinipoolManager.sol 4: import "./Base.sol";
File: contracts/contract/MultisigManager.sol 4: import "./Base.sol";
File: contracts/contract/Ocyticus.sol 4: import "./Base.sol";
File: contracts/contract/Oracle.sol 4: import "./Base.sol";
File: contracts/contract/ProtocolDAO.sol 4: import "./Base.sol";
File: contracts/contract/RewardsPool.sol 4: import "./Base.sol";
File: contracts/contract/Staking.sol 4: import "./Base.sol";
File: contracts/contract/tokens/TokenggAVAX.sol 6: import "../BaseUpgradeable.sol";
File: contracts/contract/Vault.sol 4: import "./Base.sol";
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There is 1 instance of this issue:
File: contracts/contract/BaseUpgradeable.sol 9: contract BaseUpgradeable is Initializable, BaseAbstract {
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There are 2 instances of this issue:
File: contracts/contract/Vault.sol 61: function withdrawAVAX(uint256 amount) external onlyRegisteredNetworkContract nonReentrant { 141: ) external onlyRegisteredNetworkContract nonReentrant {
override function arguments that are unused should have the variable name removed or commented out to avoid compiler warningsThere is 1 instance of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 255: function _authorizeUpgrade(address newImplementation) internal override onlyGuardian {}
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 2 instances of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit 365 560: return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit 14 76: rewardsCycleLength = 14 days;
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There is 1 instance of this issue:
File: contracts/contract/Storage.sol 41 function setGuardian(address newAddress) external { 42 // Check tx comes from current guardian 43 if (msg.sender != guardian) { 44 revert MustBeGuardian(); 45 } 46 // Store new address awaiting confirmation 47 newGuardian = newAddress; 48: }
This should especially be done if the new value is not required to be different from the old value
There are 2 instances of this issue:
File: contracts/contract/MultisigManager.sol /// @audit registerMultisig() 50: emit RegisteredMultisig(addr, msg.sender);
File: contracts/contract/Oracle.sol /// @audit setGGPPriceInAVAX() 67: emit GGPPriceUpdated(price, timestamp);
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There is 1 instance of this issue:
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol 2: pragma solidity >=0.8.0;
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There is 1 instance of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 2: pragma solidity >=0.8.0;
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 2 instances of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit seen in contracts/contract/ClaimNodeOp.sol 78: ERC20 public immutable ggp;
File: contracts/contract/Staking.sol /// @audit seen in contracts/contract/MinipoolManager.sol 58: ERC20 public immutable ggp;
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There is 1 instance of this issue:
File: contracts/contract/ProtocolDAO.sol 41: setUint(keccak256("ProtocolDAO.InflationIntervalRate"), 1000133680617113500); // 5% annual calculated on a daily interval - Calculate in js example: let dailyInflation = web3.utils.toBN((1 + 0.05) ** (1 / (365)) * 1e18);
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 2 instances of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 43: uint256 internal INITIAL_CHAIN_ID; 45: bytes32 internal INITIAL_DOMAIN_SEPARATOR;
>/>= without specifying an upper bound is unsafeThere will be breaking changes in future versions of solidity, and at that point your code will no longer be compatable. While you may have the specific version to use in a configuration file, others that include your source files may not.
There are 2 instances of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 2: pragma solidity >=0.8.0;
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol 2: pragma solidity >=0.8.0;
There are 3 instances of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit avalanchego 47: minipool.item<index>.avaxTotalRewardAmt = Actual total avax rewards paid by avalanchego to the TSS P-chain addr
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit exectued 67: // The constructor is exectued only when creating implementation contract
File: contracts/contract/Vault.sol /// @audit withdrawl 68: // Emit the withdrawl event for that contract
There are 3 instances of this issue:
File: contracts/contract/BaseUpgradeable.sol
File: contracts/contract/tokens/TokenGGP.sol
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol
There are 27 instances of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit Missing: '@return' 544 545 /// @notice Calculates how much GGP should be slashed given an expected avaxRewardAmt 546 /// @param avaxRewardAmt The amount of AVAX that should have been awarded to the validator by Avalanche 547: function calculateGGPSlashAmt(uint256 avaxRewardAmt) public view returns (uint256) { /// @audit Missing: '@return' 569 570 /// @notice Gets the minipool information from the node ID 571 /// @param nodeID 20-byte Avalanche node ID 572: function getMinipoolByNodeID(address nodeID) public view returns (Minipool memory mp) {
File: contracts/contract/ProtocolDAO.sol /// @audit Missing: '@return' 58 59 /// @notice Get if a contract is paused 60 /// @param contractName The contract that is being checked 61: function getContractPaused(string memory contractName) public view returns (bool) { /// @audit Missing: '@param claimingContract' 99 100 /// @notice The percentage a contract is owed for a rewards cycle 101 /// @return uint256 Rewards percentage a contract will receive this cycle 102: function getClaimingContractPct(string memory claimingContract) public view returns (uint256) {
File: contracts/contract/Staking.sol /// @audit Missing: '@return' 77 78 /// @notice The amount of GGP a given staker is staking 79 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 80: function getGGPStake(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param amount' 84 85 /// @notice Increase the amount of GGP a given staker is staking 86 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 87: function increaseGGPStake(address stakerAddr, uint256 amount) internal { /// @audit Missing: '@param amount' 91 92 /// @notice Decrease the amount of GGP a given staker is staking 93 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 94: function decreaseGGPStake(address stakerAddr, uint256 amount) internal { /// @audit Missing: '@return' 100 101 /// @notice The amount of AVAX a given staker is staking 102 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 103: function getAVAXStake(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param amount' 107 108 /// @notice Increase the amount of AVAX a given staker is staking 109 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 110: function increaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit Missing: '@param amount' 114 115 /// @notice Decrease the amount of AVAX a given staker is staking 116 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 117: function decreaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit Missing: '@return' 123 124 /// @notice The amount of AVAX a given staker is assigned by the protocol (for minipool creation) 125 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 126: function getAVAXAssigned(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param amount' 130 131 /// @notice Increase the amount of AVAX a given staker is assigned by the protocol (for minipool creation) 132 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 133: function increaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit Missing: '@param amount' 137 138 /// @notice Decrease the amount of AVAX a given staker is assigned by the protocol (for minipool creation) 139 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 140: function decreaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit Missing: '@return' 146 147 /// @notice Largest total AVAX amt assigned to a staker during a rewards period 148 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 149: function getAVAXAssignedHighWater(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param amount' 153 154 /// @notice Increase the AVAXAssignedHighWater 155 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 156: function increaseAVAXAssignedHighWater(address stakerAddr, uint256 amount) public onlyRegisteredNetworkContract { /// @audit Missing: '@return' 170 171 /// @notice The number of minipools the given staker has 172 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 173: function getMinipoolCount(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@return' 193 194 /// @notice The timestamp when the staker registered for GGP rewards 195 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 196: function getRewardsStartTime(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param time' 200 201 /// @notice Set the timestamp when the staker registered for GGP rewards 202 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 203 // TODO cant use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) since we also call from increaseMinipoolCount. Wat do? 204: function setRewardsStartTime(address stakerAddr, uint256 time) public onlyRegisteredNetworkContract { /// @audit Missing: '@return' 214 215 /// @notice The amount of GGP rewards the staker has earned and not claimed 216 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 217: function getGGPRewards(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@param amount' 221 222 /// @notice Increase the amount of GGP rewards the staker has earned and not claimed 223 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 224: function increaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit Missing: '@param amount' 228 229 /// @notice Decrease the amount of GGP rewards the staker has earned and not claimed 230 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 231: function decreaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit Missing: '@return' 237 238 /// @notice The most recent reward cycle number that the staker has been paid out for 239 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 240: function getLastRewardsCycleCompleted(address stakerAddr) public view returns (uint256) { /// @audit Missing: '@return' 305 306 /// @notice GGP that will count towards rewards this cycle 307 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 308: function getEffectiveGGPStaked(address stakerAddr) external view returns (uint256) { /// @audit Missing: '@return' 384 385 /// @notice Verifying the staker exists in the protocol 386 /// @param stakerAddr The C-chain address of a GGP staker in the protocol 387: function requireValidStaker(address stakerAddr) public view returns (int256) { /// @audit Missing: '@param stakerAddr' 395 396 /// @notice Get index of the staker 397 /// @return staker index or -1 if the value was not found 398: function getIndexOf(address stakerAddr) public view returns (int256) {
File: contracts/contract/Vault.sol /// @audit Missing: '@return' 190 191 /// @notice Get the AVAX balance held by a network contract 192 /// @param networkContractName Name of the contract who's AVAX balance is being requested 193: function balanceOf(string memory networkContractName) external view returns (uint256) { /// @audit Missing: '@return' 197 /// @notice Get the balance of a token held by a network contract 198 /// @param networkContractName Name of the contract who's token balance is being requested 199 /// @param tokenAddress address of the ERC20 token 200: function balanceOfToken(string memory networkContractName, ERC20 tokenAddress) external view returns (uint256) {
Consider changing the variable to be an unnamed one
There is 1 instance of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit mp 572: function getMinipoolByNodeID(address nodeID) public view returns (Minipool memory mp) {
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 15 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit setRewardsCycleTotal() came earlier 46: function isEligible(address stakerAddr) external view returns (bool) {
File: contracts/contract/MinipoolManager.sol /// @audit requireValidStateTransition() came earlier 177 constructor( 178 Storage storageAddress, 179 ERC20 ggp_, 180 TokenggAVAX ggAVAX_ 181: ) Base(storageAddress) {
File: contracts/contract/ProtocolDAO.sol /// @audit setClaimingContractPct() came earlier 115: function getInflationIntervalRate() external view returns (uint256) { /// @audit getMinCollateralizationRatio() came earlier 181: function getTargetGGAVAXReserveRate() external view returns (uint256) { /// @audit unregisterContract() came earlier 209 function upgradeExistingContract( 210 address newAddr, 211 string memory newName, 212 address existingAddr 213: ) external onlyGuardian {
File: contracts/contract/RewardsPool.sol /// @audit inflate() came earlier 105: function getRewardsCycleCount() public view returns (uint256) { /// @audit increaseRewardsCycleCount() came earlier 115: function getRewardsCycleStartTime() public view returns (uint256) { /// @audit canStartRewardsCycle() came earlier 156 function startRewardsCycle() external { 157: if (!canStartRewardsCycle()) {
File: contracts/contract/Staking.sol /// @audit decreaseGGPStake() came earlier 103: function getAVAXStake(address stakerAddr) public view returns (uint256) { /// @audit getEffectiveRewardsRatio() came earlier 308: function getEffectiveGGPStaked(address stakerAddr) external view returns (uint256) { /// @audit _stakeGGP() came earlier 358: function withdrawGGP(uint256 amount) external whenNotPaused { /// @audit getStaker() came earlier 420: function getStakers(uint256 offset, uint256 limit) external view returns (Staker[] memory stakers) {
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit initialize() came earlier 82 receive() external payable { 83: assert(msg.sender == address(asset));
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol /// @audit __ERC20Upgradeable_init() came earlier 70: function approve(address spender, uint256 amount) public virtual returns (bool) {
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol /// @audit __ERC4626Upgradeable_init() came earlier 42: function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 9 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit event GGPRewardsClaimed came earlier 27: ERC20 public immutable ggp;
File: contracts/contract/MinipoolManager.sol /// @audit event MinipoolStatusChanged came earlier 78: ERC20 public immutable ggp;
File: contracts/contract/MultisigManager.sol /// @audit event RegisteredMultisig came earlier 27: uint256 public constant MULTISIG_LIMIT = 10;
File: contracts/contract/Staking.sol /// @audit event GGPWithdrawn came earlier 56: uint256 internal constant TENTH = 0.1 ether;
File: contracts/contract/Storage.sol /// @audit event GuardianChanged came earlier 15 mapping(bytes32 => address) private addressStorage; 16 mapping(bytes32 => bool) private booleanStorage; 17 mapping(bytes32 => bytes) private bytesStorage; 18 mapping(bytes32 => bytes32) private bytes32Storage; 19 mapping(bytes32 => int256) private intStorage; 20 mapping(bytes32 => string) private stringStorage; 21: mapping(bytes32 => uint256) private uintStorage;
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit event DepositedFromStaking came earlier 41: uint32 public lastSync;
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol /// @audit event Approval came earlier 23: string public name;
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol /// @audit event Withdraw came earlier 27: ERC20 public asset;
File: contracts/contract/Vault.sol /// @audit event TokenWithdrawn came earlier 37: mapping(string => uint256) private avaxBalances;
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Lā01] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
| [Lā02] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 155 |
Total: 156 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Return values of approve() not checked | 2 |
| [Nā02] | public functions not called by the contract should be declared external instead | 54 |
| [Nā03] | Event is missing indexed fields | 26 |
Total: 82 instances over 3 issues
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: contracts/contract/Storage.sol /// @audit (valid but excluded finding) 47: newGuardian = newAddress;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There are 155 instances of this issue:
File: contracts/contract/BaseAbstract.sol /// @audit (valid but excluded finding) 25: if (getBool(keccak256(abi.encodePacked("contract.exists", msg.sender))) == false) { /// @audit (valid but excluded finding) 33: if (contractAddress != getAddress(keccak256(abi.encodePacked("contract.address", contractName)))) { /// @audit (valid but excluded finding) 41: bool isContract = getBool(keccak256(abi.encodePacked("contract.exists", msg.sender))); /// @audit (valid but excluded finding) 52: bool isContract = contractAddress == getAddress(keccak256(abi.encodePacked("contract.address", contractName))); /// @audit (valid but excluded finding) 71: int256 multisigIndex = int256(getUint(keccak256(abi.encodePacked("multisig.index", msg.sender)))) - 1; /// @audit (valid but excluded finding) 72: address addr = getAddress(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".address"))); /// @audit (valid but excluded finding) 73: bool enabled = (addr != address(0)) && getBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled"))); /// @audit (valid but excluded finding) 83: if (getBool(keccak256(abi.encodePacked("contract.paused", contractName)))) { /// @audit (valid but excluded finding) 91: address contractAddress = getAddress(keccak256(abi.encodePacked("contract.address", contractName))); /// @audit (valid but excluded finding) 100: string memory contractName = getString(keccak256(abi.encodePacked("contract.name", contractAddress)));
File: contracts/contract/MinipoolManager.sol /// @audit (valid but excluded finding) 116: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); /// @audit (valid but excluded finding) 130: address assignedMultisig = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr"))); /// @audit (valid but excluded finding) 153: bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")); /// @audit (valid but excluded finding) 246: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0); /// @audit (valid but excluded finding) 250: setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1)); /// @audit (valid but excluded finding) 251: setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID); /// @audit (valid but excluded finding) 256: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch)); /// @audit (valid but excluded finding) 257: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration); /// @audit (valid but excluded finding) 258: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".delegationFee")), delegationFee); /// @audit (valid but excluded finding) 259: setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender); /// @audit (valid but excluded finding) 260: setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig); /// @audit (valid but excluded finding) 261: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt")), msg.value); /// @audit (valid but excluded finding) 262: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value); /// @audit (valid but excluded finding) 263: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), avaxAssignmentRequest); /// @audit (valid but excluded finding) 291: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished)); /// @audit (valid but excluded finding) 293: uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 294: uint256 avaxNodeOpRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt"))); /// @audit (valid but excluded finding) 317: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 328: uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 329: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 335: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Launched)); /// @audit (valid but excluded finding) 360: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Staking)); /// @audit (valid but excluded finding) 361: setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".txID")), txID); /// @audit (valid but excluded finding) 362: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".startTime")), startTime); /// @audit (valid but excluded finding) 365: uint256 initialStartTime = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime"))); /// @audit (valid but excluded finding) 367: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), startTime); /// @audit (valid but excluded finding) 370: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); /// @audit (valid but excluded finding) 371: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 393: uint256 startTime = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".startTime"))); /// @audit (valid but excluded finding) 398: uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 399: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 405: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); /// @audit (valid but excluded finding) 407: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Withdrawable)); /// @audit (valid but excluded finding) 408: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".endTime")), endTime); /// @audit (valid but excluded finding) 409: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxTotalRewardAmt")), avaxTotalRewardAmt); /// @audit (valid but excluded finding) 420: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")), avaxNodeOpRewardAmt); /// @audit (valid but excluded finding) 421: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerRewardAmt")), avaxLiquidStakerRewardAmt); /// @audit (valid but excluded finding) 451: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt); /// @audit (valid but excluded finding) 452: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt); /// @audit (valid but excluded finding) 475: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch)); /// @audit (valid but excluded finding) 488: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); /// @audit (valid but excluded finding) 489: uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 490: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 496: setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode); /// @audit (valid but excluded finding) 497: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Error)); /// @audit (valid but excluded finding) 498: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxTotalRewardAmt")), 0); /// @audit (valid but excluded finding) 499: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")), 0); /// @audit (valid but excluded finding) 500: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerRewardAmt")), 0); /// @audit (valid but excluded finding) 522: setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode); /// @audit (valid but excluded finding) 531: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished)); /// @audit (valid but excluded finding) 567: return int256(getUint(keccak256(abi.encodePacked("minipool.index", nodeID)))) - 1; /// @audit (valid but excluded finding) 582: mp.nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID"))); /// @audit (valid but excluded finding) 583: mp.status = getUint(keccak256(abi.encodePacked("minipool.item", index, ".status"))); /// @audit (valid but excluded finding) 584: mp.duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration"))); /// @audit (valid but excluded finding) 585: mp.delegationFee = getUint(keccak256(abi.encodePacked("minipool.item", index, ".delegationFee"))); /// @audit (valid but excluded finding) 586: mp.owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner"))); /// @audit (valid but excluded finding) 587: mp.multisigAddr = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".multisigAddr"))); /// @audit (valid but excluded finding) 588: mp.avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 589: mp.avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 590: mp.txID = getBytes32(keccak256(abi.encodePacked("minipool.item", index, ".txID"))); /// @audit (valid but excluded finding) 591: mp.initialStartTime = getUint(keccak256(abi.encodePacked("minipool.item", index, ".initialStartTime"))); /// @audit (valid but excluded finding) 592: mp.startTime = getUint(keccak256(abi.encodePacked("minipool.item", index, ".startTime"))); /// @audit (valid but excluded finding) 593: mp.endTime = getUint(keccak256(abi.encodePacked("minipool.item", index, ".endTime"))); /// @audit (valid but excluded finding) 594: mp.avaxTotalRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxTotalRewardAmt"))); /// @audit (valid but excluded finding) 595: mp.errorCode = getBytes32(keccak256(abi.encodePacked("minipool.item", index, ".errorCode"))); /// @audit (valid but excluded finding) 596: mp.avaxNodeOpInitialAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpInitialAmt"))); /// @audit (valid but excluded finding) 597: mp.avaxNodeOpRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpRewardAmt"))); /// @audit (valid but excluded finding) 598: mp.avaxLiquidStakerRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerRewardAmt"))); /// @audit (valid but excluded finding) 599: mp.ggpSlashAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt"))); /// @audit (valid but excluded finding) 648: setUint(keccak256(abi.encodePacked("minipool.item", index, ".status")), uint256(MinipoolStatus.Canceled)); /// @audit (valid but excluded finding) 650: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner"))); /// @audit (valid but excluded finding) 651: uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpAmt"))); /// @audit (valid but excluded finding) 652: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 671: address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID"))); /// @audit (valid but excluded finding) 672: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner"))); /// @audit (valid but excluded finding) 673: uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration"))); /// @audit (valid but excluded finding) 674: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt"))); /// @audit (valid but excluded finding) 677: setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt); /// @audit (valid but excluded finding) 688: setBytes32(keccak256(abi.encodePacked("minipool.item", index, ".txID")), 0); /// @audit (valid but excluded finding) 689: setUint(keccak256(abi.encodePacked("minipool.item", index, ".startTime")), 0); /// @audit (valid but excluded finding) 690: setUint(keccak256(abi.encodePacked("minipool.item", index, ".endTime")), 0); /// @audit (valid but excluded finding) 691: setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxTotalRewardAmt")), 0); /// @audit (valid but excluded finding) 692: setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpRewardAmt")), 0); /// @audit (valid but excluded finding) 693: setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerRewardAmt")), 0); /// @audit (valid but excluded finding) 694: setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), 0); /// @audit (valid but excluded finding) 695: setBytes32(keccak256(abi.encodePacked("minipool.item", index, ".errorCode")), 0);
File: contracts/contract/MultisigManager.sol /// @audit (valid but excluded finding) 45: setAddress(keccak256(abi.encodePacked("multisig.item", index, ".address")), addr); /// @audit (valid but excluded finding) 48: setUint(keccak256(abi.encodePacked("multisig.index", addr)), index + 1); /// @audit (valid but excluded finding) 61: setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), true); /// @audit (valid but excluded finding) 74: setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), false); /// @audit (valid but excluded finding) 97: return int256(getUint(keccak256(abi.encodePacked("multisig.index", addr)))) - 1; /// @audit (valid but excluded finding) 110: addr = getAddress(keccak256(abi.encodePacked("multisig.item", index, ".address"))); /// @audit (valid but excluded finding) 111: enabled = (addr != address(0)) && getBool(keccak256(abi.encodePacked("multisig.item", index, ".enabled")));
File: contracts/contract/ProtocolDAO.sol /// @audit (valid but excluded finding) 62: return getBool(keccak256(abi.encodePacked("contract.paused", contractName))); /// @audit (valid but excluded finding) 68: setBool(keccak256(abi.encodePacked("contract.paused", contractName)), true); /// @audit (valid but excluded finding) 74: setBool(keccak256(abi.encodePacked("contract.paused", contractName)), false); /// @audit (valid but excluded finding) 103: return getUint(keccak256(abi.encodePacked("ProtocolDAO.ClaimingContractPct.", claimingContract))); /// @audit (valid but excluded finding) 108: setUint(keccak256(abi.encodePacked("ProtocolDAO.ClaimingContractPct.", claimingContract)), decimal); /// @audit (valid but excluded finding) 191: setBool(keccak256(abi.encodePacked("contract.exists", addr)), true); /// @audit (valid but excluded finding) 192: setAddress(keccak256(abi.encodePacked("contract.address", name)), addr); /// @audit (valid but excluded finding) 193: setString(keccak256(abi.encodePacked("contract.name", addr)), name); /// @audit (valid but excluded finding) 200: deleteBool(keccak256(abi.encodePacked("contract.exists", addr))); /// @audit (valid but excluded finding) 201: deleteAddress(keccak256(abi.encodePacked("contract.address", name))); /// @audit (valid but excluded finding) 202: deleteString(keccak256(abi.encodePacked("contract.name", addr)));
File: contracts/contract/Staking.sol /// @audit (valid but excluded finding) 82: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked"))); /// @audit (valid but excluded finding) 89: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount); /// @audit (valid but excluded finding) 96: subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount); /// @audit (valid but excluded finding) 105: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxStaked"))); /// @audit (valid but excluded finding) 112: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxStaked")), amount); /// @audit (valid but excluded finding) 119: subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxStaked")), amount); /// @audit (valid but excluded finding) 128: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned"))); /// @audit (valid but excluded finding) 135: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned")), amount); /// @audit (valid but excluded finding) 142: subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned")), amount); /// @audit (valid but excluded finding) 151: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssignedHighWater"))); /// @audit (valid but excluded finding) 158: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssignedHighWater")), amount); /// @audit (valid but excluded finding) 165: uint256 currAVAXAssigned = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned"))); /// @audit (valid but excluded finding) 166: setUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssignedHighWater")), currAVAXAssigned); /// @audit (valid but excluded finding) 175: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".minipoolCount"))); /// @audit (valid but excluded finding) 182: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".minipoolCount")), 1); /// @audit (valid but excluded finding) 189: subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".minipoolCount")), 1); /// @audit (valid but excluded finding) 198: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".rewardsStartTime"))); /// @audit (valid but excluded finding) 210: setUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".rewardsStartTime")), time); /// @audit (valid but excluded finding) 219: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpRewards"))); /// @audit (valid but excluded finding) 226: addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpRewards")), amount); /// @audit (valid but excluded finding) 233: subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpRewards")), amount); /// @audit (valid but excluded finding) 242: return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".lastRewardsCycleCompleted"))); /// @audit (valid but excluded finding) 250: setUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".lastRewardsCycleCompleted")), cycleNumber); /// @audit (valid but excluded finding) 350: setUint(keccak256(abi.encodePacked("staker.index", stakerAddr)), uint256(stakerIndex + 1)); /// @audit (valid but excluded finding) 351: setAddress(keccak256(abi.encodePacked("staker.item", stakerIndex, ".stakerAddr")), stakerAddr); /// @audit (valid but excluded finding) 399: return int256(getUint(keccak256(abi.encodePacked("staker.index", stakerAddr)))) - 1; /// @audit (valid but excluded finding) 406: staker.ggpStaked = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked"))); /// @audit (valid but excluded finding) 407: staker.avaxAssigned = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned"))); /// @audit (valid but excluded finding) 408: staker.avaxStaked = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxStaked"))); /// @audit (valid but excluded finding) 409: staker.stakerAddr = getAddress(keccak256(abi.encodePacked("staker.item", stakerIndex, ".stakerAddr"))); /// @audit (valid but excluded finding) 410: staker.minipoolCount = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".minipoolCount"))); /// @audit (valid but excluded finding) 411: staker.rewardsStartTime = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".rewardsStartTime"))); /// @audit (valid but excluded finding) 412: staker.ggpRewards = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpRewards"))); /// @audit (valid but excluded finding) 413: staker.lastRewardsCycleCompleted = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".lastRewardsCycleCompleted")));
File: contracts/contract/Storage.sol /// @audit (valid but excluded finding) 29: if (booleanStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))] == false && msg.sender != guardian) {
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit (valid but excluded finding) 59: if (amt > 0 && getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) { /// @audit (valid but excluded finding) 207: if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) { /// @audit (valid but excluded finding) 217: if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
File: contracts/contract/Vault.sol /// @audit (valid but excluded finding) 124: bytes32 contractKey = keccak256(abi.encodePacked(networkContractName, address(tokenContract))); /// @audit (valid but excluded finding) 179: bytes32 contractKeyTo = keccak256(abi.encodePacked(networkContractName, tokenAddress)); /// @audit (valid but excluded finding) 201: return tokenBalances[keccak256(abi.encodePacked(networkContractName, tokenAddress))];
approve() not checkedNot all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
There are 2 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit (valid but excluded finding) 105: ggp.approve(address(staking), restakeAmt);
File: contracts/contract/Staking.sol /// @audit (valid but excluded finding) 342: ggp.approve(address(vault), amount);
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 54 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit (valid but excluded finding) 40: function setRewardsCycleTotal(uint256 amount) public onlySpecificRegisteredContract("RewardsPool", msg.sender) {
File: contracts/contract/MinipoolManager.sol /// @audit (valid but excluded finding) 541: function getTotalAVAXLiquidStakerAmt() public view returns (uint256) { /// @audit (valid but excluded finding) 572: function getMinipoolByNodeID(address nodeID) public view returns (Minipool memory mp) { /// @audit (valid but excluded finding) 607 function getMinipools( 608 MinipoolStatus status, 609 uint256 offset, 610 uint256 limit 611: ) public view returns (Minipool[] memory minipools) { /// @audit (valid but excluded finding) 634: function getMinipoolCount() public view returns (uint256) {
File: contracts/contract/MultisigManager.sol /// @audit (valid but excluded finding) 102: function getCount() public view returns (uint256) {
File: contracts/contract/ProtocolDAO.sol /// @audit (valid but excluded finding) 61: function getContractPaused(string memory contractName) public view returns (bool) { /// @audit (valid but excluded finding) 67: function pauseContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { /// @audit (valid but excluded finding) 73: function resumeContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { /// @audit (valid but excluded finding) 81: function getRewardsEligibilityMinSeconds() public view returns (uint256) { /// @audit (valid but excluded finding) 86: function getRewardsCycleSeconds() public view returns (uint256) { /// @audit (valid but excluded finding) 91: function getTotalGGPCirculatingSupply() public view returns (uint256) { /// @audit (valid but excluded finding) 96: function setTotalGGPCirculatingSupply(uint256 amount) public onlySpecificRegisteredContract("RewardsPool", msg.sender) { /// @audit (valid but excluded finding) 102: function getClaimingContractPct(string memory claimingContract) public view returns (uint256) { /// @audit (valid but excluded finding) 107: function setClaimingContractPct(string memory claimingContract, uint256 decimal) public onlyGuardian valueNotGreaterThanOne(decimal) { /// @audit (valid but excluded finding) 123: function getInflationIntervalSeconds() public view returns (uint256) { /// @audit (valid but excluded finding) 130: function getMinipoolMinAVAXStakingAmt() public view returns (uint256) { /// @audit (valid but excluded finding) 135: function getMinipoolNodeCommissionFeePct() public view returns (uint256) { /// @audit (valid but excluded finding) 140: function getMinipoolMaxAVAXAssignment() public view returns (uint256) { /// @audit (valid but excluded finding) 145: function getMinipoolMinAVAXAssignment() public view returns (uint256) { /// @audit (valid but excluded finding) 150: function getMinipoolCancelMoratoriumSeconds() public view returns (uint256) { /// @audit (valid but excluded finding) 156: function setExpectedAVAXRewardsRate(uint256 rate) public onlyMultisig valueNotGreaterThanOne(rate) { /// @audit (valid but excluded finding) 161: function getExpectedAVAXRewardsRate() public view returns (uint256) { /// @audit (valid but excluded finding) 168: function getMaxCollateralizationRatio() public view returns (uint256) { /// @audit (valid but excluded finding) 173: function getMinCollateralizationRatio() public view returns (uint256) {
File: contracts/contract/RewardsPool.sol /// @audit (valid but excluded finding) 105: function getRewardsCycleCount() public view returns (uint256) {
File: contracts/contract/Staking.sol /// @audit (valid but excluded finding) 66: function getTotalGGPStake() public view returns (uint256) { /// @audit (valid but excluded finding) 103: function getAVAXStake(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 110: function increaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 117: function decreaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 133: function increaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 140: function decreaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 156: function increaseAVAXAssignedHighWater(address stakerAddr, uint256 amount) public onlyRegisteredNetworkContract { /// @audit (valid but excluded finding) 163: function resetAVAXAssignedHighWater(address stakerAddr) public onlyRegisteredNetworkContract { /// @audit (valid but excluded finding) 173: function getMinipoolCount(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 180: function increaseMinipoolCount(address stakerAddr) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 187: function decreaseMinipoolCount(address stakerAddr) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 196: function getRewardsStartTime(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 204: function setRewardsStartTime(address stakerAddr, uint256 time) public onlyRegisteredNetworkContract { /// @audit (valid but excluded finding) 217: function getGGPRewards(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 224: function increaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit (valid but excluded finding) 231: function decreaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit (valid but excluded finding) 240: function getLastRewardsCycleCompleted(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 248: function setLastRewardsCycleCompleted(address stakerAddr, uint256 cycleNumber) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit (valid but excluded finding) 256: function getMinimumGGPStake(address stakerAddr) public view returns (uint256) { /// @audit (valid but excluded finding) 328: function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { /// @audit (valid but excluded finding) 379: function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit (valid but excluded finding) 72: function initialize(Storage storageAddress, ERC20 asset) public initializer { /// @audit (valid but excluded finding) 88 function syncRewards() public { 89: uint32 timestamp = block.timestamp.safeCastTo32(); /// @audit (valid but excluded finding) 142: function depositFromStaking(uint256 baseAmt, uint256 rewardAmt) public payable onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 153: function withdrawForStaking(uint256 assets) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { /// @audit (valid but excluded finding) 166: function depositAVAX() public payable returns (uint256 shares) { /// @audit (valid but excluded finding) 180: function withdrawAVAX(uint256 assets) public returns (uint256 shares) { /// @audit (valid but excluded finding) 191: function redeemAVAX(uint256 shares) public returns (uint256 assets) {
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 26 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit (valid but excluded finding) 25: event GGPRewardsClaimed(address indexed to, uint256 amount);
File: contracts/contract/ClaimProtocolDAO.sol /// @audit (valid but excluded finding) 13: event GGPTokensSentByDAOProtocol(string invoiceID, address indexed from, address indexed to, uint256 amount);
File: contracts/contract/MinipoolManager.sol /// @audit (valid but excluded finding) 75: event GGPSlashed(address indexed nodeID, uint256 ggp);
File: contracts/contract/MultisigManager.sol /// @audit (valid but excluded finding) 23: event DisabledMultisig(address indexed multisig, address actor); /// @audit (valid but excluded finding) 24: event EnabledMultisig(address indexed multisig, address actor); /// @audit (valid but excluded finding) 25: event RegisteredMultisig(address indexed multisig, address actor);
File: contracts/contract/Oracle.sol /// @audit (valid but excluded finding) 21: event GGPPriceUpdated(uint256 indexed price, uint256 timestamp);
File: contracts/contract/RewardsPool.sol /// @audit (valid but excluded finding) 24: event GGPInflated(uint256 newTokens); /// @audit (valid but excluded finding) 25: event NewRewardsCycleStarted(uint256 totalRewardsAmt); /// @audit (valid but excluded finding) 26: event ClaimNodeOpRewardsTransfered(uint256 value); /// @audit (valid but excluded finding) 27: event ProtocolDAORewardsTransfered(uint256 value); /// @audit (valid but excluded finding) 28: event MultisigRewardsTransfered(uint256 value);
File: contracts/contract/Staking.sol /// @audit (valid but excluded finding) 40: event GGPStaked(address indexed from, uint256 amount); /// @audit (valid but excluded finding) 41: event GGPWithdrawn(address indexed to, uint256 amount);
File: contracts/contract/Storage.sol /// @audit (valid but excluded finding) 12: event GuardianChanged(address oldGuardian, address newGuardian);
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit (valid but excluded finding) 36: event NewRewardsCycle(uint256 indexed cycleEnd, uint256 rewardsAmt); /// @audit (valid but excluded finding) 37: event WithdrawnForStaking(address indexed caller, uint256 assets); /// @audit (valid but excluded finding) 38: event DepositedFromStaking(address indexed caller, uint256 baseAmt, uint256 rewardsAmt);
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol /// @audit (valid but excluded finding) 15: event Transfer(address indexed from, address indexed to, uint256 amount); /// @audit (valid but excluded finding) 17: event Approval(address indexed owner, address indexed spender, uint256 amount);
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol /// @audit (valid but excluded finding) 19: event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
File: contracts/contract/Vault.sol /// @audit (valid but excluded finding) 30: event AVAXDeposited(string indexed by, uint256 amount); /// @audit (valid but excluded finding) 31: event AVAXTransfer(string indexed from, string indexed to, uint256 amount); /// @audit (valid but excluded finding) 32: event AVAXWithdrawn(string indexed by, uint256 amount); /// @audit (valid but excluded finding) 33: event TokenDeposited(bytes32 indexed by, address indexed tokenAddress, uint256 amount); /// @audit (valid but excluded finding) 35: event TokenWithdrawn(bytes32 indexed by, address indexed tokenAddress, uint256 amount);
#0 - GalloDaSballo
2023-01-25T19:14:19Z
| [Lā01] | Inflation not locked for four years | 1 | R Code as is will revert after 4 years, so it's enforced via config
| [Lā02] | Contract will stop functioning in the year 2106 | 1 | L
| [Lā03] | Lower-level initializations should come first | 1 | R in lack of specific risk
| [Lā04] | Cycle end may be be too soon | 1 | I don't think this is valid
| [Lā05] | Incorrect percentage conversion | 1 | L
| [Lā06] | Missing checks for value of msg.value | 1 | Invalid: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L401
| [Lā07] | Loss of precision | 2 | L
| [Lā08] | Signatures vulnerable to malleability attacks | 1 | L | [Lā09] | require() should be used instead of assert() | 1 | R
| [Lā10] | Open TODOs | 1 | NC
| [Nā01] | Common code should be refactored | 1 | R
| [Nā02] | String constants used in multiple places should be defined as constants | 1 | R
| [Nā03] | Constants in comparisons should appear on the left side | 1 | R
| [Nā04] | Inconsistent address separator in storage names | 1 | NC
| [Nā05] | Confusing function name | 1 | NC
| [Nā06] | Misplaced punctuation | 1 | NC
| [Nā07] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 1 | Disputing for TokenAvax as it's the child contract
| [Nā08] | Import declarations should import specific identifiers, rather than the whole file | 13 | NC
| [Nā09] | Missing initializer modifier on constructor | 1 | R
| [Nā10] | The nonReentrant modifier should occur before all other modifiers | 2 | R
| [Nā11] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 1 | NC
| [Nā12] | constants should be defined rather than using magic numbers | 2 | R
| [Nā13] | Missing event and or timelock for critical parameter change | 1 | NC
| [Nā14] | Events that mark critical parameter changes should contain both the old and the new value | 2 | NC
| [Nā15] | Use a more recent version of solidity | 1 | NC
| [Nā16] | Use a more recent version of solidity | 1 | See N-15
| [Nā17] | Constant redefined elsewhere | 2 | R
| [Nā18] | Lines are too long | 1 | NC
| [Nā19] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 2 | R
| [Nā20] | Using >/>= without specifying an upper bound is unsafe | 2 | R
| [Nā21] | Typos | 3 | NC
| [Nā22] | File is missing NatSpec | 3 | NC
| [Nā23] | NatSpec is incomplete | 27 | NC
| [Nā24] | Not using the named return variables anywhere in the function is confusing | 1 | R
| [Nā25] | Contracts should have full test coverage | 1 | R
| [Nā26] | Large or complicated code bases should implement fuzzing tests | 1 | R
| [Nā27] | Function ordering does not follow the Solidity style guide | 15 | NC
| [Nā28] | Contract does not follow the Solidity style guide's suggested layout ordering | 9 | NC
#1 - GalloDaSballo
2023-02-03T16:33:08Z
2L from dups
4L 15R 15NC
#2 - GalloDaSballo
2023-02-03T16:33:20Z
6L 15R 15NC including dups
#3 - c4-judge
2023-02-03T22:09:06Z
GalloDaSballo marked the issue as grade-a
#4 - c4-judge
2023-02-03T22:09:13Z
GalloDaSballo marked the issue as selected for report
#5 - c4-judge
2023-02-09T09:33:19Z
GalloDaSballo marked the issue as not selected for report
#6 - c4-judge
2023-02-09T09:35:01Z
GalloDaSballo marked the issue as selected for report
#7 - GalloDaSballo
2023-02-09T09:35:35Z
Best report by far, so far I though the second best was the best (this one scores above 100%)
Well played
š Selected for report: NoamYakov
Also found by: AkshaySrivastav, IllIllI, ast3ros, betweenETHlines, c3phas, camdengrieh, chaduke, fatherOfBlocks, kartkhira, latt1ce, shark
718.9381 USDC - $718.94
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Modifier unnecessarily looks up storage variables | 2 | 4200 |
| [Gā02] | Batch pause and resume to save gas | 1 | 600 |
| [Gā03] | Cheaper to calculate than to read and update | 1 | 2100 |
| [Gā04] | Use 1 and 2 instead of bools to save gas | 1 | 17100 |
| [Gā05] | Wastes gas to clear the old newGuardian | 1 | 17100 |
| [Gā06] | Cheaper to calculate domain separator every time | 1 | 4200 |
| [Gā07] | Contract calculations having to do with msg.value can be unchecked | 1 | 60 |
| [Gā08] | State variables should be cached in stack variables rather than re-reading them from storage | 4 | 388 |
| [Gā09] | The result of function calls should be cached rather than re-calling the function | 2 | - |
| [Gā10] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 6 | 678 |
| [Gā11] | internal functions only called once can be inlined to save gas | 5 | 100 |
| [Gā12] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 7 | 420 |
| [Gā13] | keccak256() should only need to be called on a specific string literal once | 74 | 3108 |
| [Gā14] | Optimize names to save gas | 12 | 264 |
| [Gā15] | Use a more recent version of solidity | 2 | - |
| [Gā16] | String literals passed to abi.encode()/abi.encodePacked() should not be split by commas | 3 | - |
| [Gā17] | >= costs less gas than > | 2 | 6 |
| [Gā18] | Splitting require() statements that use && saves gas | 1 | 3 |
| [Gā19] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 3 | - |
| [Gā20] | Don't compare boolean expressions to boolean literals | 3 | 27 |
| [Gā21] | Stack variable used as a cheaper cache for a state variable is only used once | 1 | 3 |
| [Gā22] | Superfluous event fields | 1 | - |
| [Gā23] | Functions guaranteed to revert when called by normal users can be marked payable | 63 | 1323 |
Total: 197 instances over 23 issues with 51680 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
If the else-condition isn't required, isContract will have been looked up unnecessarily
There are 2 instances of this issue:
File: /contracts/contract/BaseAbstract.sol 40 modifier guardianOrRegisteredContract() { 41 bool isContract = getBool(keccak256(abi.encodePacked("contract.exists", msg.sender))); 42 bool isGuardian = msg.sender == gogoStorage.getGuardian(); 43 44 if (!(isGuardian || isContract)) { 45: revert MustBeGuardianOrValidContract(); 51 modifier guardianOrSpecificRegisteredContract(string memory contractName, address contractAddress) { 52 bool isContract = contractAddress == getAddress(keccak256(abi.encodePacked("contract.address", contractName))); 53 bool isGuardian = msg.sender == gogoStorage.getGuardian(); 54 55 if (!(isGuardian || isContract)) { 56: revert MustBeGuardianOrValidContract();
If pause/resumeEverything() are made to be functions on ProtocolDAO, you'll save the overhead of six external calls (100 gas each)
There is 1 instance of this issue:
File: /contracts/contract/Ocyticus.sol 37 function pauseEverything() external onlyDefender { 38 ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO")); 39 dao.pauseContract("TokenggAVAX"); 40 dao.pauseContract("MinipoolManager"); 41 dao.pauseContract("Staking"); 42 disableAllMultisigs(); 43 } 44 45 /// @notice Reestablish all contract's abilities 46 /// @dev Multisigs will need to be enabled seperately, we dont know which ones to enable 47 function resumeEverything() external onlyDefender { 48 ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO")); 49 dao.resumeContract("TokenggAVAX"); 50 dao.resumeContract("MinipoolManager"); 51 dao.resumeContract("Staking"); 52: }
The new value is known, so it's cheaper to set the new multisig count directly, rather than having addUint() look up the old value, then overwrite it
There is 1 instance of this issue:
File: /contracts/contract/MultisigManager.sol 48 setUint(keccak256(abi.encodePacked("multisig.index", addr)), index + 1); 49: addUint(keccak256("multisig.count"), 1);
1 and 2 instead of bools to save gasUsing 1 and 2 prevents the storage slot from being marked as cleared, which will save gas when later setting the value back to true, since it will already be non-zero and thus a cheaper storage write than if it had been zero
There is 1 instance of this issue:
File: /contracts/contract/Storage.sol 16: mapping(bytes32 => bool) private booleanStorage;
newGuardianClearing the value means the next time it's changed to something else, the operation is more expensive than if it had been changed from the old value
There is 1 instance of this issue:
File: /contracts/contract/Storage.sol 64: delete newGuardian;
Since INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR are no longer immutable, but are state variables, you end up looking up their value every time, which incurs a very large gas penalty. It's cheaper to just calculate it every time, or to use OpenZeppelin's version which is optimized for this case
There is 1 instance of this issue:
File: /contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 163: return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
msg.value can be uncheckedThis is because msg.value will never be more than type(uint256).max
There is 1 instance of this issue:
File: /contracts/contract/tokens/TokenggAVAX.sol 143 uint256 totalAmt = msg.value; 144: if (totalAmt != (baseAmt + rewardAmt) || baseAmt > stakingTotalAssets) {
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 4 instances of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit rewardsCycleLength on line 76 /// @audit rewardsCycleLength on line 78 78: rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength; /// @audit rewardsCycleLength on line 102 /// @audit rewardsCycleLength on line 102 102: uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
The instances below point to the second+ call of the function within a single function
There are 2 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit rewardsPool.getRewardsCycleCount() on line 61 65: if (staking.getLastRewardsCycleCompleted(stakerAddr) == rewardsPool.getRewardsCycleCount()) { /// @audit rewardsPool.getRewardsCycleCount() on line 61 68: staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 6 instances of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 149: stakingTotalAssets -= baseAmt; 160: stakingTotalAssets += assets; 245: totalReleasedAssets -= amount; 252: totalReleasedAssets += amount;
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 184: totalSupply += amount; 201: totalSupply -= amount;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 5 instances of this issue:
File: contracts/contract/RewardsPool.sol 82 function inflate() internal { 83: ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO")); 110 function increaseRewardsCycleCount() internal { 111: addUint(keccak256("RewardsPool.RewardsCycleCount"), 1); 203 function distributeMultisigAllotment( 204 uint256 allotment, 205 Vault vault, 206: TokenGGP ggp
File: contracts/contract/Staking.sol 87: function increaseGGPStake(address stakerAddr, uint256 amount) internal {
File: contracts/contract/tokens/TokenggAVAX.sol 248 function afterDeposit( 249 uint256 amount, 250: uint256 /* shares */
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 7 instances of this issue:
File: contracts/contract/MinipoolManager.sol 619: for (uint256 i = offset; i < max; i++) {
File: contracts/contract/MultisigManager.sol 84: for (uint256 i = 0; i < total; i++) {
File: contracts/contract/Ocyticus.sol 61: for (uint256 i = 0; i < count; i++) {
File: contracts/contract/RewardsPool.sol 74: for (uint256 i = 0; i < inflationIntervalsElapsed; i++) { 215: for (uint256 i = 0; i < count; i++) { 230: for (uint256 i = 0; i < enabledMultisigs.length; i++) {
File: contracts/contract/Staking.sol 428: for (uint256 i = offset; i < max; i++) {
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
There are 74 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol 36: return getUint(keccak256("NOPClaim.RewardsCycleTotal")); 41: setUint(keccak256("NOPClaim.RewardsCycleTotal"), amount);
File: contracts/contract/MinipoolManager.sol 248: minipoolIndex = int256(getUint(keccak256("minipool.count"))); 252: addUint(keccak256("minipool.count"), 1); 333: addUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt); 433: subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt); 512: subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt); 542: return getUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt")); 612: uint256 totalMinipools = getUint(keccak256("minipool.count")); 635: return getUint(keccak256("minipool.count"));
File: contracts/contract/MultisigManager.sol 40: uint256 index = getUint(keccak256("multisig.count")); 49: addUint(keccak256("multisig.count"), 1); 81: uint256 total = getUint(keccak256("multisig.count")); 103: return getUint(keccak256("multisig.count"));
File: contracts/contract/Oracle.sol 29: setAddress(keccak256("Oracle.OneInch"), addr); 38: IOneInch oneinch = IOneInch(getAddress(keccak256("Oracle.OneInch"))); 47: price = getUint(keccak256("Oracle.GGPPriceInAVAX")); 51: timestamp = getUint(keccak256("Oracle.GGPTimestamp")); 58: uint256 lastTimestamp = getUint(keccak256("Oracle.GGPTimestamp")); 65: setUint(keccak256("Oracle.GGPPriceInAVAX"), price); 66: setUint(keccak256("Oracle.GGPTimestamp"), timestamp);
File: contracts/contract/ProtocolDAO.sol 24: if (getBool(keccak256("ProtocolDAO.initialized"))) { 27: setBool(keccak256("ProtocolDAO.initialized"), true); 30: setUint(keccak256("ProtocolDAO.RewardsEligibilityMinSeconds"), 14 days); 33: setUint(keccak256("ProtocolDAO.RewardsCycleSeconds"), 28 days); // The time in which a claim period will span in seconds - 28 days by default 34: setUint(keccak256("ProtocolDAO.TotalGGPCirculatingSupply"), 18_000_000 ether); 35: setUint(keccak256("ProtocolDAO.ClaimingContractPct.ClaimMultisig"), 0.20 ether); 36: setUint(keccak256("ProtocolDAO.ClaimingContractPct.ClaimNodeOp"), 0.70 ether); 37: setUint(keccak256("ProtocolDAO.ClaimingContractPct.ClaimProtocolDAO"), 0.10 ether); 40: setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days); 41: setUint(keccak256("ProtocolDAO.InflationIntervalRate"), 1000133680617113500); // 5% annual calculated on a daily interval - Calculate in js example: let dailyInflation = web3.utils.toBN((1 + 0.05) ** (1 / (365)) * 1e18); 44: setUint(keccak256("ProtocolDAO.TargetGGAVAXReserveRate"), 0.1 ether); // 10% collateral held in reserve 47: setUint(keccak256("ProtocolDAO.MinipoolMinAVAXStakingAmt"), 2_000 ether); 48: setUint(keccak256("ProtocolDAO.MinipoolNodeCommissionFeePct"), 0.15 ether); 49: setUint(keccak256("ProtocolDAO.MinipoolMaxAVAXAssignment"), 10_000 ether); 50: setUint(keccak256("ProtocolDAO.MinipoolMinAVAXAssignment"), 1_000 ether); 51: setUint(keccak256("ProtocolDAO.ExpectedAVAXRewardsRate"), 0.1 ether); // Annual rate as pct of 1 avax 52: setUint(keccak256("ProtocolDAO.MinipoolCancelMoratoriumSeconds"), 5 days); 55: setUint(keccak256("ProtocolDAO.MaxCollateralizationRatio"), 1.5 ether); 56: setUint(keccak256("ProtocolDAO.MinCollateralizationRatio"), 0.1 ether); 82: return getUint(keccak256("ProtocolDAO.RewardsEligibilityMinSeconds")); 87: return getUint(keccak256("ProtocolDAO.RewardsCycleSeconds")); 92: return getUint(keccak256("ProtocolDAO.TotalGGPCirculatingSupply")); 97: return setUint(keccak256("ProtocolDAO.TotalGGPCirculatingSupply"), amount); 117: uint256 rate = getUint(keccak256("ProtocolDAO.InflationIntervalRate")); 124: return getUint(keccak256("ProtocolDAO.InflationIntervalSeconds")); 131: return getUint(keccak256("ProtocolDAO.MinipoolMinAVAXStakingAmt")); 136: return getUint(keccak256("ProtocolDAO.MinipoolNodeCommissionFeePct")); 141: return getUint(keccak256("ProtocolDAO.MinipoolMaxAVAXAssignment")); 146: return getUint(keccak256("ProtocolDAO.MinipoolMinAVAXAssignment")); 151: return getUint(keccak256("ProtocolDAO.MinipoolCancelMoratoriumSeconds")); 157: setUint(keccak256("ProtocolDAO.ExpectedAVAXRewardsRate"), rate); 162: return getUint(keccak256("ProtocolDAO.ExpectedAVAXRewardsRate")); 169: return getUint(keccak256("ProtocolDAO.MaxCollateralizationRatio")); 174: return getUint(keccak256("ProtocolDAO.MinCollateralizationRatio")); 182: return getUint(keccak256("ProtocolDAO.TargetGGAVAXReserveRate"));
File: contracts/contract/RewardsPool.sol 35: if (getBool(keccak256("RewardsPool.initialized"))) { 38: setBool(keccak256("RewardsPool.initialized"), true); 40: setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp); 41: setUint(keccak256("RewardsPool.InflationIntervalStartTime"), block.timestamp); 49: return getUint(keccak256("RewardsPool.InflationIntervalStartTime")); 98: addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds); 99: setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens); 106: return getUint(keccak256("RewardsPool.RewardsCycleCount")); 111: addUint(keccak256("RewardsPool.RewardsCycleCount"), 1); 116: return getUint(keccak256("RewardsPool.RewardsCycleStartTime")); 121: return getUint(keccak256("RewardsPool.RewardsCycleTotalAmt")); 164: setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp);
File: contracts/contract/Staking.sol 73: return getUint(keccak256("staker.count")); 348: stakerIndex = int256(getUint(keccak256("staker.count"))); 349: addUint(keccak256("staker.count"), 1);
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 139: keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"), 170: keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), 172: keccak256("1"),
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 12 instances of this issue:
File: contracts/contract/ClaimNodeOp.sol /// @audit getRewardsCycleTotal(), setRewardsCycleTotal(), isEligible(), calculateAndDistributeRewards(), claimAndRestake() 17: contract ClaimNodeOp is Base {
File: contracts/contract/ClaimProtocolDAO.sol /// @audit spend() 10: contract ClaimProtocolDAO is Base {
File: contracts/contract/MinipoolManager.sol /// @audit receiveWithdrawalAVAX(), createMinipool(), cancelMinipool(), withdrawMinipoolFunds(), canClaimAndInitiateStaking(), claimAndInitiateStaking(), recordStakingStart(), recordStakingEnd(), recreateMinipool(), recordStakingError(), cancelMinipoolByMultisig(), finishFailedMinipoolByMultisig(), getTotalAVAXLiquidStakerAmt(), calculateGGPSlashAmt(), getExpectedAVAXRewardsAmt(), getIndexOf(), getMinipoolByNodeID(), getMinipool(), getMinipools(), getMinipoolCount() 57: contract MinipoolManager is Base, ReentrancyGuard, IWithdrawer {
File: contracts/contract/MultisigManager.sol /// @audit registerMultisig(), enableMultisig(), disableMultisig(), requireNextActiveMultisig(), getIndexOf(), getCount(), getMultisig() 17: contract MultisigManager is Base {
File: contracts/contract/Ocyticus.sol /// @audit addDefender(), removeDefender(), pauseEverything(), resumeEverything(), disableAllMultisigs() 10: contract Ocyticus is Base {
File: contracts/contract/Oracle.sol /// @audit setOneInch(), getGGPPriceInAVAXFromOneInch(), getGGPPriceInAVAX(), setGGPPriceInAVAX() 17: contract Oracle is Base {
File: contracts/contract/ProtocolDAO.sol /// @audit initialize(), getContractPaused(), pauseContract(), resumeContract(), getRewardsEligibilityMinSeconds(), getRewardsCycleSeconds(), getTotalGGPCirculatingSupply(), setTotalGGPCirculatingSupply(), getClaimingContractPct(), setClaimingContractPct(), getInflationIntervalRate(), getInflationIntervalSeconds(), getMinipoolMinAVAXStakingAmt(), getMinipoolNodeCommissionFeePct(), getMinipoolMaxAVAXAssignment(), getMinipoolMinAVAXAssignment(), getMinipoolCancelMoratoriumSeconds(), setExpectedAVAXRewardsRate(), getExpectedAVAXRewardsRate(), getMaxCollateralizationRatio(), getMinCollateralizationRatio(), getTargetGGAVAXReserveRate(), registerContract(), unregisterContract(), upgradeExistingContract() 9: contract ProtocolDAO is Base {
File: contracts/contract/RewardsPool.sol /// @audit initialize(), getInflationIntervalStartTime(), getInflationIntervalsElapsed(), getInflationAmt(), getRewardsCycleCount(), getRewardsCycleStartTime(), getRewardsCycleTotalAmt(), getRewardsCyclesElapsed(), getClaimingContractDistribution(), canStartRewardsCycle(), startRewardsCycle() 15: contract RewardsPool is Base {
File: contracts/contract/Staking.sol /// @audit getTotalGGPStake(), getStakerCount(), getGGPStake(), getAVAXStake(), increaseAVAXStake(), decreaseAVAXStake(), getAVAXAssigned(), increaseAVAXAssigned(), decreaseAVAXAssigned(), getAVAXAssignedHighWater(), increaseAVAXAssignedHighWater(), resetAVAXAssignedHighWater(), getMinipoolCount(), increaseMinipoolCount(), decreaseMinipoolCount(), getRewardsStartTime(), setRewardsStartTime(), getGGPRewards(), increaseGGPRewards(), decreaseGGPRewards(), getLastRewardsCycleCompleted(), setLastRewardsCycleCompleted(), getMinimumGGPStake(), getCollateralizationRatio(), getEffectiveRewardsRatio(), getEffectiveGGPStaked(), stakeGGP(), restakeGGP(), withdrawGGP(), slashGGP(), requireValidStaker(), getIndexOf(), getStaker(), getStakers() 30: contract Staking is Base {
File: contracts/contract/Storage.sol /// @audit setGuardian(), getGuardian(), confirmGuardian(), getAddress(), getBool(), getBytes(), getBytes32(), getInt(), getString(), getUint(), setAddress(), setBool(), setBytes(), setBytes32(), setInt(), setString(), setUint(), deleteAddress(), deleteBool(), deleteBytes(), deleteBytes32(), deleteInt(), deleteString(), deleteUint(), addUint(), subUint() 7: contract Storage {
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit initialize(), syncRewards(), amountAvailableForStaking(), depositFromStaking(), withdrawForStaking(), depositAVAX(), withdrawAVAX(), redeemAVAX() 24: contract TokenggAVAX is Initializable, ERC4626Upgradeable, UUPSUpgradeable, BaseUpgradeable {
File: contracts/contract/Vault.sol /// @audit depositAVAX(), withdrawAVAX(), transferAVAX(), depositToken(), withdrawToken(), transferToken(), balanceOf(), balanceOfToken(), addAllowedToken(), removeAllowedToken() 19: contract Vault is Base, ReentrancyGuard {
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 2 instances of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 2: pragma solidity >=0.8.0;
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol 2: pragma solidity >=0.8.0;
abi.encode()/abi.encodePacked() should not be split by commasString literals can be split into multiple parts and still be considered as a single string literal. Adding commas between each chunk makes it no longer a single string, and instead multiple strings. EACH new comma costs 21 gas due to stack operations and separate MSTOREs
There are 3 instances of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit 1 commas 59: if (amt > 0 && getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) { /// @audit 1 commas 207: if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) { /// @audit 1 commas 217: if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
>= costs less gas than >The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas
There are 2 instances of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 212: return assets > avail ? avail : assets; 222: return shares > avail ? avail : shares;
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas
There is 1 instance of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 154: require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contractĆ¢ā¬ā¢s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
There are 3 instances of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol /// @audit uint32 rewardsCycleLength 76: rewardsCycleLength = 14 days; /// @audit uint32 rewardsCycleEnd 78: rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength; /// @audit uint192 lastRewardsAmt 104: lastRewardsAmt = nextRewardsAmt.safeCastTo192();
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There are 3 instances of this issue:
File: contracts/contract/BaseAbstract.sol 25: if (getBool(keccak256(abi.encodePacked("contract.exists", msg.sender))) == false) { 74: if (enabled == false) {
File: contracts/contract/Storage.sol 29: if (booleanStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))] == false && msg.sender != guardian) {
If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend
There is 1 instance of this issue:
File: contracts/contract/tokens/TokenggAVAX.sol 97: uint256 stakingTotalAssets_ = stakingTotalAssets;
block.timestamp and block.number are added to event information by default so adding them manually wastes gas
There is 1 instance of this issue:
File: contracts/contract/Oracle.sol 21: event GGPPriceUpdated(uint256 indexed price, uint256 timestamp);
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 63 instances of this issue:
File: contracts/contract/BaseUpgradeable.sol 10: function __BaseUpgradeable_init(Storage gogoStorageAddress) internal onlyInitializing {
File: contracts/contract/ClaimNodeOp.sol 40: function setRewardsCycleTotal(uint256 amount) public onlySpecificRegisteredContract("RewardsPool", msg.sender) { 56: function calculateAndDistributeRewards(address stakerAddr, uint256 totalEligibleGGPStaked) external onlyMultisig {
File: contracts/contract/ClaimProtocolDAO.sol 20 function spend( 21 string memory invoiceID, 22 address recipientAddress, 23 uint256 amount 24: ) external onlyGuardian {
File: contracts/contract/MultisigManager.sol 35: function registerMultisig(address addr) external onlyGuardian { 55: function enableMultisig(address addr) external onlyGuardian {
File: contracts/contract/Ocyticus.sol 27: function addDefender(address defender) external onlyGuardian { 32: function removeDefender(address defender) external onlyGuardian { 37: function pauseEverything() external onlyDefender { 47: function resumeEverything() external onlyDefender { 55: function disableAllMultisigs() public onlyDefender {
File: contracts/contract/Oracle.sol 28: function setOneInch(address addr) external onlyGuardian { 57: function setGGPPriceInAVAX(uint256 price, uint256 timestamp) external onlyMultisig {
File: contracts/contract/ProtocolDAO.sol 23: function initialize() external onlyGuardian { 67: function pauseContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { 73: function resumeContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { 96: function setTotalGGPCirculatingSupply(uint256 amount) public onlySpecificRegisteredContract("RewardsPool", msg.sender) { 107: function setClaimingContractPct(string memory claimingContract, uint256 decimal) public onlyGuardian valueNotGreaterThanOne(decimal) { 156: function setExpectedAVAXRewardsRate(uint256 rate) public onlyMultisig valueNotGreaterThanOne(rate) { 190: function registerContract(address addr, string memory name) public onlyGuardian { 198: function unregisterContract(address addr) public onlyGuardian { 209 function upgradeExistingContract( 210 address newAddr, 211 string memory newName, 212 address existingAddr 213: ) external onlyGuardian {
File: contracts/contract/RewardsPool.sol 34: function initialize() external onlyGuardian {
File: contracts/contract/Staking.sol 110: function increaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 117: function decreaseAVAXStake(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 133: function increaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 140: function decreaseAVAXAssigned(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 156: function increaseAVAXAssignedHighWater(address stakerAddr, uint256 amount) public onlyRegisteredNetworkContract { 163: function resetAVAXAssignedHighWater(address stakerAddr) public onlyRegisteredNetworkContract { 180: function increaseMinipoolCount(address stakerAddr) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 187: function decreaseMinipoolCount(address stakerAddr) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 204: function setRewardsStartTime(address stakerAddr, uint256 time) public onlyRegisteredNetworkContract { 224: function increaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { 231: function decreaseGGPRewards(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { 248: function setLastRewardsCycleCompleted(address stakerAddr, uint256 cycleNumber) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { 328: function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) { 379: function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
File: contracts/contract/Storage.sol 104: function setAddress(bytes32 key, address value) external onlyRegisteredNetworkContract { 108: function setBool(bytes32 key, bool value) external onlyRegisteredNetworkContract { 112: function setBytes(bytes32 key, bytes calldata value) external onlyRegisteredNetworkContract { 116: function setBytes32(bytes32 key, bytes32 value) external onlyRegisteredNetworkContract { 120: function setInt(bytes32 key, int256 value) external onlyRegisteredNetworkContract { 124: function setString(bytes32 key, string calldata value) external onlyRegisteredNetworkContract { 128: function setUint(bytes32 key, uint256 value) external onlyRegisteredNetworkContract { 136: function deleteAddress(bytes32 key) external onlyRegisteredNetworkContract { 140: function deleteBool(bytes32 key) external onlyRegisteredNetworkContract { 144: function deleteBytes(bytes32 key) external onlyRegisteredNetworkContract { 148: function deleteBytes32(bytes32 key) external onlyRegisteredNetworkContract { 152: function deleteInt(bytes32 key) external onlyRegisteredNetworkContract { 156: function deleteString(bytes32 key) external onlyRegisteredNetworkContract { 160: function deleteUint(bytes32 key) external onlyRegisteredNetworkContract { 170: function addUint(bytes32 key, uint256 amount) external onlyRegisteredNetworkContract { 176: function subUint(bytes32 key, uint256 amount) external onlyRegisteredNetworkContract {
File: contracts/contract/tokens/TokenggAVAX.sol 153: function withdrawForStaking(uint256 assets) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) { 255: function _authorizeUpgrade(address newImplementation) internal override onlyGuardian {}
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol 53 function __ERC20Upgradeable_init( 54 string memory _name, 55 string memory _symbol, 56 uint8 _decimals 57: ) internal onlyInitializing {
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol 29 function __ERC4626Upgradeable_init( 30 ERC20 _asset, 31 string memory _name, 32 string memory _symbol 33: ) internal onlyInitializing {
File: contracts/contract/Vault.sol 61: function withdrawAVAX(uint256 amount) external onlyRegisteredNetworkContract nonReentrant { 84: function transferAVAX(string memory toContractName, uint256 amount) external onlyRegisteredNetworkContract { 137 function withdrawToken( 138 address withdrawalAddress, 139 ERC20 tokenAddress, 140 uint256 amount 141: ) external onlyRegisteredNetworkContract nonReentrant { 166 function transferToken( 167 string memory networkContractName, 168 ERC20 tokenAddress, 169 uint256 amount 170: ) external onlyRegisteredNetworkContract { 204: function addAllowedToken(address tokenAddress) external onlyGuardian { 208: function removeAllowedToken(address tokenAddress) external onlyGuardian {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Using calldata instead of memory for read-only arguments in external functions saves gas | 12 | 1440 |
| [Gā02] | State variables should be cached in stack variables rather than re-reading them from storage | 2 | 194 |
| [Gā03] | <array>.length should not be looked up in every loop of a for-loop | 1 | 3 |
| [Gā04] | Using bools for storage incurs overhead | 3 | 51300 |
| [Gā05] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 10 | 50 |
| [Gā06] | Using private rather than public for constants, saves gas | 1 | - |
| [Gā07] | Division by two should use bit shifting | 1 | 20 |
| [Gā08] | Use custom errors rather than revert()/require() strings to save gas | 4 | - |
Total: 34 instances over 8 issues with 53007 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 12 instances of this issue:
File: contracts/contract/ClaimProtocolDAO.sol /// @audit invoiceID - (valid but excluded finding) 20 function spend( 21 string memory invoiceID, 22 address recipientAddress, 23 uint256 amount 24: ) external onlyGuardian {
File: contracts/contract/ProtocolDAO.sol /// @audit contractName - (valid but excluded finding) 61: function getContractPaused(string memory contractName) public view returns (bool) { /// @audit contractName - (valid but excluded finding) 67: function pauseContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { /// @audit contractName - (valid but excluded finding) 73: function resumeContract(string memory contractName) public onlySpecificRegisteredContract("Ocyticus", msg.sender) { /// @audit claimingContract - (valid but excluded finding) 102: function getClaimingContractPct(string memory claimingContract) public view returns (uint256) { /// @audit claimingContract - (valid but excluded finding) 107: function setClaimingContractPct(string memory claimingContract, uint256 decimal) public onlyGuardian valueNotGreaterThanOne(decimal) { /// @audit newName - (valid but excluded finding) 209 function upgradeExistingContract( 210 address newAddr, 211 string memory newName, 212 address existingAddr 213: ) external onlyGuardian {
File: contracts/contract/Vault.sol /// @audit toContractName - (valid but excluded finding) 84: function transferAVAX(string memory toContractName, uint256 amount) external onlyRegisteredNetworkContract { /// @audit networkContractName - (valid but excluded finding) 108 function depositToken( 109 string memory networkContractName, 110 ERC20 tokenContract, 111 uint256 amount 112: ) external guardianOrRegisteredContract { /// @audit networkContractName - (valid but excluded finding) 166 function transferToken( 167 string memory networkContractName, 168 ERC20 tokenAddress, 169 uint256 amount 170: ) external onlyRegisteredNetworkContract { /// @audit networkContractName - (valid but excluded finding) 193: function balanceOf(string memory networkContractName) external view returns (uint256) { /// @audit networkContractName - (valid but excluded finding) 200: function balanceOfToken(string memory networkContractName, ERC20 tokenAddress) external view returns (uint256) {
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 2 instances of this issue:
File: contracts/contract/Storage.sol /// @audit newGuardian on line 57 - (valid but excluded finding) 63: guardian = newGuardian; /// @audit guardian on line 63 - (valid but excluded finding) 65: emit GuardianChanged(oldGuardian, guardian);
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There is 1 instance of this issue:
File: contracts/contract/RewardsPool.sol /// @audit (valid but excluded finding) 230: for (uint256 i = 0; i < enabledMultisigs.length; i++) {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 3 instances of this issue:
File: contracts/contract/Ocyticus.sol /// @audit (valid but excluded finding) 13: mapping(address => bool) public defenders;
File: contracts/contract/Storage.sol /// @audit (valid but excluded finding) 16: mapping(bytes32 => bool) private booleanStorage;
File: contracts/contract/Vault.sol /// @audit (valid but excluded finding) 39: mapping(address => bool) private allowedTokens;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 10 instances of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit (valid but excluded finding) 619: for (uint256 i = offset; i < max; i++) { /// @audit (valid but excluded finding) 623: total++;
File: contracts/contract/MultisigManager.sol /// @audit (valid but excluded finding) 84: for (uint256 i = 0; i < total; i++) {
File: contracts/contract/Ocyticus.sol /// @audit (valid but excluded finding) 61: for (uint256 i = 0; i < count; i++) {
File: contracts/contract/RewardsPool.sol /// @audit (valid but excluded finding) 74: for (uint256 i = 0; i < inflationIntervalsElapsed; i++) { /// @audit (valid but excluded finding) 215: for (uint256 i = 0; i < count; i++) { /// @audit (valid but excluded finding) 219: enabledCount++; /// @audit (valid but excluded finding) 230: for (uint256 i = 0; i < enabledMultisigs.length; i++) {
File: contracts/contract/Staking.sol /// @audit (valid but excluded finding) 428: for (uint256 i = offset; i < max; i++) { /// @audit (valid but excluded finding) 431: total++;
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: contracts/contract/MultisigManager.sol /// @audit (valid but excluded finding) 27: uint256 public constant MULTISIG_LIMIT = 10;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There is 1 instance of this issue:
File: contracts/contract/MinipoolManager.sol /// @audit (valid but excluded finding) 413: uint256 avaxHalfRewards = avaxTotalRewardAmt / 2;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 4 instances of this issue:
File: contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol /// @audit (valid but excluded finding) 127: require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED"); /// @audit (valid but excluded finding) 154: require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");
File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol /// @audit (valid but excluded finding) 44: require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES"); /// @audit (valid but excluded finding) 103: require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
#0 - GalloDaSballo
2023-01-25T19:31:44Z
| [Gā01] | Modifier unnecessarily looks up storage variables | 2 | 4200 | 4.2k
| [Gā02] | Batch pause and resume to save gas | 1 | 600 | 600
| [Gā03] | Cheaper to calculate than to read and update | 1 | 2100 | 100 (it's already hot)
| [Gā04] | Use 1 and 2 instead of bools to save gas | 1 | 17100 | Will not award, but it's a great saving for the Sponsor IMO
| [Gā05] | Wastes gas to clear the old newGuardian | 1 | 17100 | See G-04 as well
| [Gā06] | Cheaper to calculate domain separator every time | 1 | 4200 | Let's say 2.1k gas
| [Gā07] | Contract calculations having to do with msg.value can be unchecked | 1 | 60 | 20
| [Gā08] | State variables should be cached in stack variables rather than re-reading them from storage | 4 | 388 | 400
| [Gā09] | The result of function calls should be cached rather than re-calling the function | 2 | - | 340 * 2 680
Rest are marginal, let's say 300 gas
8400
#1 - GalloDaSballo
2023-02-03T18:16:00Z
Btw check these for += it's not that impactful https://gist.github.com/GalloDaSballo/8521d2cb6dcc4eea5523a06957f0ea15
#2 - c4-judge
2023-02-03T19:12:01Z
GalloDaSballo marked the issue as grade-a
#3 - c4-judge
2023-02-03T19:12:11Z
GalloDaSballo marked the issue as selected for report
#4 - c4-judge
2023-02-03T19:12:39Z
GalloDaSballo marked the issue as grade-b
#5 - c4-judge
2023-02-03T19:12:46Z
GalloDaSballo marked the issue as grade-a
#6 - c4-judge
2023-02-08T08:06:52Z
GalloDaSballo marked the issue as not selected for report