Platform: Code4rena
Start Date: 01/07/2021
Pot Size: $100,000 USDC
Total HM: 10
Participants: 7
Period: 7 days
Judge: ghoulsol
Total Solo HM: 4
Id: 17
League: ETH
Rank: 2/7
Findings: 6
Award: $23,682.33
🌟 Selected for report: 27
🚀 Solo Findings: 1
3531.5985 USDC - $3,531.60
0xRajeev
The distributeStrategyGainLoss() function distributes any gains or losses generated from a harvest and is expected to be called only by valid protocol vault adaptors. It is an externally visible function and the access control is indirectly enforced on msg.sender by checking that vaultIndexes[msg.sender] is a valid index range 1-4. However the operator used in the require() is || instead of &&, which allows an arbitrary msg.sender, i.e. attacker, to bypass the check.
Scenario: An arbitrary non-vault address calling this function will get an index of 0 because of default mapping value in vaultIndexes[msg.sender] which will fail the > 0 check but pass the <= N_COINS + 1 check (N_COINS = 3) because 0 <= 4 which will allow control to go past this check. Furthermore, on L362, index=0 will underflow the -1 decrement (due to lack of SafeMath.sub and use of < 0.8.0 solc) and index will be set to (uint256_MAX - 1). This will allow execution to proceed to the else part of conditional meant for curve LP vault. Therefore, this will allow any random address to call this function with arbitrary values of gain/loss and distribute arbitrary gain/loss appearing to come from Curve vault.
The attack control flow: -> Controller.distributeStrategyGainLoss(ARBITRARY_HIGH_VALUE_OF_GAIN, 0) -> index = 0 passes check for the index <= N_COINS + 1 part of predicate on L357 in Controller.sol -> index = uint256_MAX after L362 -> gainUsd = ibuoy.lpToUsd(ARBITRARY_HIGH_VALUE_OF_GAIN); on L371 in Controller.sol -> ipnl.distributeStrategyGainLoss(gainUsd, lossUsd, reward); on L376 in Controller.sol -> (gvtAssets, pwrdAssets, performanceBonus) = handleInvestGain(lastGA, lastPA, gain, reward); on L254 in PnL.sol -> performanceBonus = profit.mul(performanceFee).div(PERCENTAGE_DECIMAL_FACTOR); on L186 of PnL.sol -> gvt.mint(reward, gvt.factor(gvtAssets), performanceBonus); on L256 in PnL.sol
Manual Analysis
Change || to && in require() on L357 of Controller.sol to prevent arbitrary addresses from going past this check. Or consider explicit access control for the authorized vault adaptors.
0xRajeev
The contracts use Chainlink’s deprecated API latestAnswer(). Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs.
Impact: Deprecated API stops working. Prices cannot be obtained. Protocol stops and contracts have to be redeployed.
See similar Low-severity finding L11 from OpenZeppelin's Audit of Opyn Gamma Protocol: https://blog.openzeppelin.com/opyn-gamma-protocol-audit/
See https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference/#latestanswer.
Manual Analysis
Use V3 interface functions: https://docs.chain.link/docs/price-feeds-api-reference/
#0 - flabble-gro
2021-07-14T20:23:04Z
Duplicate of #106
#1 - ghoul-sol
2021-07-26T01:30:51Z
Duplicate of #106 which means it's a medium risk.
1765.7993 USDC - $1,765.80
0xRajeev
The addSafeAddress() takes an address and adds it to a “safe list". This is used in eoaOnly() to give exemption to safe addresses that are trusted smart contracts, when all other smart contacts are prevented from protocol interaction. The stated purpose is to allow only such partner/trusted smart contract integrations (project rep mentioned Argent wallet as the only one for now but that may change) an exemption from potential flash loan threats. But if there a safe listed integration that needs to be later disabled, it cannot be done. The protocol will have to rely on other measures (outside the scope of this contest) to prevent flash loan manipulations which are specified as an area of critical concern.
Scenario: A trusted integration/partner address is added to safe list. But that wallet/protocol/DApp is later manipulated (by project, its users or an attacker) to somehow launch a flash loan attack on the protocol. However, its address cannot be removed from the safe list and the protocol cannot prevent flash loan manipulations from that source because of its exemption. Contract/project will have to be redeployed.
Manual Analysis
Change addSafeAddress() to isSafeAddress() with an additional bool parameter to allow both enabling/disabling of safe addresses.
#0 - kitty-the-kat
2021-07-14T16:39:44Z
low risk - Made specifically for one partner in beta period, and planned to be removed. We added the removal function for sanity.
#1 - ghoul-sol
2021-07-26T02:36:00Z
I'll keep medium risk because this could put the protocol into a one way street and not being able to remove safe addresses is quite dangerous. Medium risk.
🌟 Selected for report: 0xRajeev
3923.9983 USDC - $3,924.00
0xRajeev
The switchEoaOnly() allows the owner to disable preventSmartContracts (the project’s plan apparently is to do so after the beta-period) which will allow any smart contract to interact with the protocol and potentially exploit any underlying flash loan vulnerabilities which are specified as an area of critical concern.
The current mitigation is to optionally prevent contracts, except whitelisted partner ones, from interacting with the protocol to prevent any flash loan manipulations. A more robust approach is to add logic to prevent multiple txs to protocol from the same address/tx.origin within the same block when smart contracts are allowed. This will avoid any reliance on trust with integrating partners/protocols.
Manual Analysis
Add logic to prevent multiple txs to protocol from the same address within the same block.
#0 - kitty-the-kat
2021-07-14T16:35:17Z
Low-severity: This is a temporary blocker to not let SCs interact with gro-protocol, planned to be removed after beta as it might potentially stop other integrations (as per issue 51)
#1 - ghoul-sol
2021-07-26T13:25:14Z
It looks like a low risk issue since it's a future problem and not something that is an immediate issue, however, it's not clear how the protocol will protect itself against flash loans after this temporary blocker is off. One of the critical protocol's concerns are flash loans manipulations therefore I think medium risk is justified here.
588.5998 USDC - $588.60
0xRajeev
Missing emits for declared events indicate potentially missing logic, redundant declarations or reduced off-chain monitoring capabilities.
Scenario: For example, the event LogFlashSwitchUpdated is missing an emit in Controller. Based on the name, this is presumably related to flash loans being enabled/disabled which could have significant security implications. Or the (misspelled) LogHealhCheckUpdate which is presumably related to a health check logic that is missing in LifeGuard.
Manual Analysis
Evaluate if logic is missing and add logic+emit or remove event.
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
The design choice seems to allow a whitelist of addresses (bots or trusted parties) that can trigger pause/emergency but onlyOwner can unpause/restart (and perform other privileged functions). While it is recommended in general to have separate privileges/roles for stopping and starting critical functions, having only a single owner for unpause/restart triggering may create a single point of failure if owner is EOA and keys are lost/compromised.
Scenario: Protocol is paused or put in emergency mode by a bot/user in whitelist. Owner is an EOA and the private keys are lost. Protocol cannot be unpaused/restarted.
Manual Analysis
Evaluate this design choice to see if a whitelist should also be allowed to unpause/restart. At a minimum, use a 6-of-9 or higher multisig and not an EOA.
#0 - kitty-the-kat
2021-07-14T16:47:47Z
Multi sig planned
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
Uninitialized system/curve vaults (default to zero address) will lead to reverts on calls and expect owner to set them before other functions are called because functions do not check if system has been initialized. This requires a robust deployment script which is fail-safe.
The same applies to many other address parameters in the protocol e.g.: reward.
Scenario: All vaults are not initialized because of a script error or an admin mistake. Protocol goes live and user interactions result in exceptions. Users lose trust and protocol reputation takes a hit.
Manual Analysis
Evaluate non-zero defaults, initializing from constructor or maintaining/checking an initialization state variable which prevents other functions from being called until all critical system states such as vault addresses are initialized.
#0 - kitty-the-kat
2021-07-14T16:40:11Z
Controller by gro governance, dealt with in deployment scripts
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
The addSafeAddress() takes an address and adds it to a “safe list". This is used in eoaOnly() to give exemption to safe addresses that are trusted smart contracts, when all other smart contacts are prevented from protocol interaction. The stated purpose is to allow only such partner/trusted smart contract integrations (project rep mentioned Argent wallet as the only one for now but that may change) an exemption from potential flash loan threats.
The eoaOnly() check used during deposits and withdrawals checks if preventSmartContracts is enabled and if so, makes sure the transaction is coming from an integration/partner smart contract. But instead of using msg.sender in the check it uses tx.origin. This is suspect because tx.origin gives the originating EOA address of the transaction and not a smart contract’s address. (This may get even more complicated with the proposed EIP-3074.)
Discussion with the project team indicated that this is indeed not the norm but is apparently the case for their only current (none others planned) integration with Argent wallet where the originating account is Argent’s relayer tx.origin i.e. flow: Argent relayer (tx.origin) => Argent user wallet (msg.sender) => gro protocol while the typically expected flow is: user EOA (tx.origin) => proxy (msg.sender) => gro protocol.
While this has reportedly been verified and tested, it does seem strange and perhaps warrants a re-evaluation because the exemption for this/other trusted integration/partner smart contracts will not work otherwise.
Scenario: Partner contract is added to the safe address for exemption but the integration fails because of the use of tx.origin instead of msg.sender.
Manual Analysis
Re-evaluate the use of tx.origin instead of msg.sender.
#0 - kitty-the-kat
2021-07-14T16:32:37Z
Made for specific partner, so cannot be generic
While this has reportedly been verified and tested, it does seem strange and perhaps warrants a re-evaluation because the exemption for this/other trusted integration/partner smart contracts will not work otherwise.
This SC protection is only temporary as we are aware of EIP-3074, and consideration of how we need to change this/or if we need this at all are ongoing.
0xRajeev
Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations of some constructors.
Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.
Manual Analysis
Add zero-address checks for all initializations/setters of all address state variables.
#0 - flabble-gro
2021-07-14T20:22:31Z
Duplicate of #90
#1 - ghoul-sol
2021-07-26T01:22:36Z
Duplicate of #90
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
Token amounts/prices are typically open-ranged and inclusive of the bounds. Using ‘<‘ or ‘>’ instead of ‘<=‘ and ‘>=‘ may affect borderline scenarios, considered unintuitive by users and affect accounting.
Scenario 1: In calculateVaultSwapData(), the require() check is: require(withdrawAmount < state.totalCurrentAssetsUsd, "Withdrawal exceeds system assets"); The ‘<‘ could be replaced by ‘<=‘
Scenario 2: In withdrawSingleByLiquidity(), the require() check is: require(balance > minAmount, "withdrawSingle: !minAmount"); The ‘>’ should be ‘>=‘ as is used in the similar check in withdrawSingleByExchange().
Manual Analysis
Reconsider strict inequalities and relax them if possible.
#0 - kitty-the-kat
2021-07-14T16:21:49Z
We haven't been able to model an exploit for this
588.5998 USDC - $588.60
0xRajeev
vaultIndexes is uninitialized and it's unclear what 10000 signifies here. investDelta return value is also ignored at call site. If this is an indication of missed/incorrect logic, then it's risky. If not, removing will help readability/maintainability.
Manual Analysis
Evaluate any missing logic or else remove unused code.
588.5998 USDC - $588.60
0xRajeev
BaseVaultAdaptor contains logic that is “built on top of any vault in order for it to function with Gro protocol.” One of such functions is the migrate() function which is onlyOwner and takes an address parameter which allows owner to migrate vault’s entire balance at any time to that address. This is extremely risky because it gives an opportunity for, at least a perception of, rug-pull by a disgruntled/malicious owner/dev to the protocol users/community. This could also be dangerous if triggered accidentally especially by an EOA owner address or maliciously via compromised keys.
Scenario1: Protocol launches and starts accumulating TVL. A savvy user analyzes source and shares the presence of this migrate() function as potential owner rug-pull vector. Users withdraw funds and protocol reputation takes a hit.
Scenario 2: Protocol launches and hits 100MM TVL. A disgruntled dev/owner migrates vault assets to their address and drains the protocol.
Scenario 3: Protocol launches and hits 100MM TVL. Owner EOA keys get compromised and attacker migrates vault assets to their address and drains the protocol.
See similar concern on migrate() functionality in ShibaSwap recently: Yearn dev https://twitter.com/bantg/status/1412370758987354116 https://twitter.com/bantg/status/1412388385663164425 Others https://twitter.com/valentinmihov/status/1412352490918625280 https://twitter.com/shegenerates/status/1412642215537545218
Manual Analysis
Evaluate the need for this function and avoid/mitigate risk appropriately.
#0 - kitty-the-kat
2021-07-15T11:41:18Z
Low risk
#1 - ghoul-sol
2021-07-26T14:41:20Z
Agree with sponsor. Assuming malicious behavior of owner is low risk if it's a governance and timelock is used. Low risk.
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
preventSmartContracts is initialized to false which allows users to deposit/withdraw funds from the protocol via (custom) smart contracts because the eoaOnly check during deposits/withdrawals always succeeds. However, if protocol owner decides to suddenly enable preventSmartContracts then smart contracts are prevented from interaction unless they are exempted in safe addresses.
The lack of an event in switchEoaOnly() to inform off-chain monitors/interfaces about the enabling/disabling, say from false -> true, and lack of a time-delayed enforcement of this prevention of contracts from depositing/withdrawing causes users who have previously deposited via smart contracts (that are not safeAddresses) to get locked out of withdrawals leading to fund lock/loss.
Scenario: User deposits funds via smart contract (not in safe address list) when preventSmartContracts=false. Protocol owner sets preventSmartContracts=true. User’s funds are locked/lost in protocol.
Manual Analysis
Add event + time-delayed enforcement to switchEoaOnly() so users can monitor and withdraw funds deposited via smart contracts.
#0 - kitty-the-kat
2021-07-14T16:30:11Z
Low criticality/Not an issue - Workaround exists (safe addresses)
#1 - ghoul-sol
2021-07-26T15:35:03Z
Agree with sponsor. While the scenario is correct, it all comes down to the management of the protocol. From different context I also assume that this option will be set to true for beta and safe addresses will be whitelisted. I'm making this a low risk because this can create too many angry users to be non-critical.
🌟 Selected for report: 0xRajeev
1307.9994 USDC - $1,308.00
0xRajeev
The need for an externally visible rebalanceTrigger() (when rebalance() does that check itself) is apparently that the whitelisted bot checks trigger before calling the very expensive/security-sensitive rebalance() operation which again checks to see if anything has changed between then and the previous trigger.
Exposing the rebalance trigger check externally for convenience may offer a front-running arbitrage opportunity to a non-whitelisted i.e. any bot which can check when a rebalance will be triggered by a whitelisted bot and then using that information to arbitrage on underlying stablecoins/strategies, which may affect system exposure.
Discussion with the project team reported that this is technically possible but only within the BP limit (25-50) of the current vs cached price (where the BASIS_POINTS is currently set to 20). If not, the Buoy safetyCheck will fail.
Manual Analysis
Recommend adding onlyWhitelist modifier to rebalanceTrigger() which allows retaining the convenience of bots, but only whitelisted ones, checking before calling rebalance. This makes it only a little safer because one can always front-run the actual rebalance call. This will only force bots to monitor mempool for rebalances instead of arbing ahead of time. Revisit this aspect for any missed considerations.
#0 - kitty-the-kat
2021-07-14T16:22:34Z
There is price check before rebalance. It is not very useful to add whitelist on view function. Because the code is public, anyone can implement a local function easily.
#1 - ghoul-sol
2021-07-26T15:48:28Z
Solution proposed by warden does not solve the problem but the problem still remains valid. The issue looks quite generic and it's really a MEV problem that most protocols have. For that reason, I think it's reasonable to degrade to low risk.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
The feeToken logic is to account for tokens that may charge transfer fees and therefore require balance checks before/after transfers. For now, the only token that is programmed to potentially do so (in future, not currently) is USDT (neither DAI/USDC have this capability).
Impact: While this flexible future-proof logic is good design, this costs 3 SLOADs = 32100 = 6300 gas for reading the state variable feeToken 3 times (different index each time i.e. costs 2100, not 100) while the only token programmed for transfer fees is USDT (which has never charged fees). 2100 gas + two external token balance calls for USDT (26002 = 5200 gas + balance gas costs) >= total of 7300 gas for USDT and 4200 gas for other two tokens is perhaps expensive to support this future-proofing logic. However, from a security-perspective, it might be safer to leave this in here for USDT but remove checking for other two.
Manual Analysis
Evaluate removing it completely or hardcoding logic only for USDT index=2 to save gas.
🌟 Selected for report: a_delamo
Also found by: 0xRajeev, GalloDaSballo, shw
38.4024 USDC - $38.40
0xRajeev
The _stableToUsd() and _stableToLp() functions receive uint256[N_COINS] memory tokenAmounts as a parameter and then copy it to another array _tokenAmounts to pass it as an argument to curvePool.calc_token_amount(). They can instead pass the parameter directly as argument to curvePool call. This will save some gas in the perf-critical deposit/withdraw flows and also improve readability.
This same patter exists in multiple places.
Manual Analysis
Avoid redundant copying to save a bit of gas and improve readability.
#0 - kitty-the-kat
2021-07-14T20:46:53Z
#27
#1 - ghoul-sol
2021-07-26T03:20:54Z
Duplicate of #27
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
The latest version of solc compiler is 8.6. Gro contracts use solc version >=0.6.0 <0.7.0, which is fairly dated. This may be a carry-over from initial versions of project to minimize porting code to handle breaking changes across solc 0.7.0 or 0.8.0.
Impact: Upgrading the solc compiler to 0.8 will give the latest compiler benefits including bug fixes, security enhancements and overall optimizations especially the in-built overflow/underflow checks which may give gas savings by avoiding expensive SafeMath.
Manual Analysis
Consider porting over code to solc >= 0.8.0 for bug fixes, security enhancements and overall optimizations for gas savings.
#0 - kitty-the-kat
2021-07-14T16:56:48Z
No plans to swap to 0.8.x
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs. Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase.
In setDependencies(), writing the addresses to local variables first and then using them both in writing to state variables and in emit will save will save 4 SLOADS worth 21004 = 8400 gas. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L65 Doing the same thing in WithdrawHandler.sol will save 5 SLOADS worth 21005 = 10500 gas. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L74-L88
Manual Analysis
Use equivalent function parameters or local variables in event emits instead of state variables.
#0 - kitty-the-kat
2021-07-14T16:56:32Z
Functions not used a lot, gas saving overtime would be low, not a priority.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
EIP-2929 in Berlin fork increased the gas costs of SLOADs and CALL* family opcodes increasing them for not-accessed slots/addresses and decreasing them for accessed slots. EIP-2930 optionally supports specifying an access list in the transition of all slots and addresses accessed by the transaction which reduces their gas cost upon access and prevents EIP-2929 gas cost increases from breaking contracts.
Impact: Considering these changes may significantly impact gas usage for transactions that call functions touching many state variables or making many external calls.
Example 1: Common user-triggered functions deposit*() and withdraw*() which presumably cost 100Ks of gas can use access lists for all the state variables they touch and addresses of calls made to reduce gas.
Example 2: Common bot-triggered functions such as rebalance*() which potentially touch many state variables and make external contract calls across the entire protocol may benefit significantly by considering the use of access lists.
https://eips.ethereum.org/EIPS/eip-2929
https://eips.ethereum.org/EIPS/eip-2930
https://hackmd.io/@fvictorio/gas-costs-after-berlin
https://github.com/gakonst/ethers-rs/issues/265
Manual Analysis
Evaluate the feasibility of using access lists to save gas due to EIPs 2929 & 2930 post-Berlin hard fork. The tooling support is WIP.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase. Successive SLOADs cost 100 gas. Memory stores/loads (MSTOREs/MLOADs) cost only 3 gas. Therefore, by caching repeatedly read state variables in local variables within a function, one can save >=100 gas.
Caching ctrl address in a local variable will save 300 gas because it is SLOADed 4 times now in this critical deposit flow. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L112 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L115 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L119 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L121
Caching lg address state variable in a local variable outside the loop can save 1100 gas by avoiding 4 unnecessary SLOADs per loop iteration (4*3 = 12 but one SLOAD is hoisted out of the loop = 11 extra SLOADS at 100 gas = 1100 gas). https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L147-L151
Caching buoy address state variable in the function beginning can save 100 gas from an extra SLOAD. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L174-L176
Caching insurance address in function beginning can save 100 gas from an unnecessary SLOAD. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L193 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L198
Caching lg address in function beginning can save 100 gas from an unnecessary SLOAD. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L197 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L199
Hoisting buoy state variable out of the loop and caching it in a local variable will save 300 gas from 3 unnecessary SLOADs. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L181
Caching buoy in a local variable will save 100 gas. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L212 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L219
Caching ctrl in a local variable at the function beginning and using that in the rest of this function will save 4 unnecessary SLOADs i.e. 400 gas in this function. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L211 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L221 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L226 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L236 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L260 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L264
Hoisting buoy out of the loop and caching in a local variable will save 3 unnecessary SLOADs and so 300 gas. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L329
Caching lg in a local variable will save 100 gas. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L356 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L357
Manual Analysis
Cache repeatedly read state variables (especially those within a loop) in local variables at an appropriate part of the function (preferably the beginning) and use them instead of state variables. Converting SLOADs to MLOADs reduces gas from 100 to 3.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
Moving declarations of state variables that take < 32 Bytes next to each other will allow combining them in the same storage slot and potentially save gas from combined SSTOREs depending on store patterns.
Impact: Moving emergencyState bool right next to preventSmartContracts bool will conserve a storage slot and may save gas.
See reference: https://mudit.blog/solidity-gas-optimization-tips/ and https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6
Manual Analysis
Moving declarations of state variables that take < 32 Bytes next to each other. E.g.: booleans next to each other or address types.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
Declarations of state variables need not unnecessarily initialize them to the type’s default values. This will save a bit of gas.
Default value of bool is 0 i.e. false: https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L44 Default value of integers is 0 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L63
Manual Analysis
Remove unnecessary initializations
#0 - kitty-the-kat
2021-07-14T16:54:54Z
acknowledged, but not a priority to fix
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
Unnecessary zero-address check for account in addReferral() because it is always msg.sender (can never be 0) in the only call site from DepositHandler::depositGToken(). Removing this check can save a little gas in the critical deposit flow.
Manual Analysis
Remove unnecessary zero-address check.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
In isValidBigFish(), the calculation of gvt and pard assets by making an external call to PnL.calcPnL() is required only if the amount is >= bigFishAbsoluteThreshold.
Impact: Moving this logic for calculation of assets to the else part where it is required will save gas due to the external pnl call (2600 call + 2*2100 SLOADs for state variable reads in calcPnL()) for the sardine flow, where this is not required.
Manual Analysis
Move logic to else part instead of doing it before the conditional as shown below:
if (amount < bigFishAbsoluteThreshold) { return false; } else if (amount > assets) { return true; } else { (uint256 gvtAssets, uint256 pwrdAssets) = IPnL(pnl).calcPnL(); uint256 assets = pwrdAssets.add(gvtAssets); return amount > assets.mul(bigFishThreshold).div(PERCENTAGE_DECIMAL_FACTOR); }
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external function’s parameters are not copied into memory and are instead read from calldata directly. If a function is called only from with that contract or derived contracts, making it internal/private can further reduce gas costs because the expensive calls are converted into cheaper jumps.
The only callers of eoaOnly() are external contracts DepositHandler and WithdrawHandler. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L268 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L112 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L211 The only caller of calcSystemTargetDelta() is Insurance. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Allocation.sol#L62-L63 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L213 The only caller of calcVaultTargetDelta() is Insurance. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Allocation.sol#L92-L93 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L432 validGTokenDecrease() can be made private just like validGTokenIncrease because it is only called from within Controller. https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L448 https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L248
Manual Analysis
Change function visibility from public to external/private where possible.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
The minAmount <= amount check in _prepareForWithdrawalSingle() is an unnecessary check because the same check has already passed in both lg.withdrawSingleByLiquidity and lg.withdrawSingleByExchange. And there is no logic that changes the checked parameters between the earlier checks and this one.
Manual Analysis
Remove unnecessary check.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
The investDelta return variable from function getVaultDeltaForDeposit() is ignored at the only call-site in DepositHandler. Removing it can save some gas.
Manual Analysis
Remove unused return value or add logic to use it at caller.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
In LG setDependencies(), the code to approve withdrawHandler to pull from lifeguard is repeated twice, once to set it to 0 allowance if the withdrawHandler is != 0 and then unconditionally to set it MAX. Given that this is the only function that sets withdrawHandler, the first set of 0 approvals seem to be redundant given the unconditional approvals that follow. Removing this can save some gas although we don’t expect this to be called often.
The redundant logic could be for the case where the withdrawHandler is updated and the old handler is given an approval of 0 and the new one MAX.
Manual Analysis
Evaluate code and remove logic if redundant. If this is present to handle withdrawHandler updates then ignore this recommendation.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
In LifeGuard3Pool (LG) deposit(), lp token balance is determined for the crv3pool.add_liquidity() call. Given that LG does not hold any lp tokens between txs, there is no need to determine and subtract lp token balance before and after the curve add liquidity call. Removing the call on L204 will save at least 2600+2100=4700 gas from the external call.
Manual Analysis
Remove the call on L204 and just get the balance on L206 without any subtraction.
🌟 Selected for report: 0xRajeev
210.7126 USDC - $210.71
0xRajeev
The for loop in investSingle() can be removed in favor of simpler logic to calculate k [k = N_COINS - (i + j)], which will save some gas in the deposit flow.
Manual Analysis
Replace L317 to L323 with:
uint256 inBalance = inAmounts[N_COINS - (i + j)]; if (inBalance > 0) { _exchange(inBalance, int128(k), int128(i)); }