Platform: Code4rena
Start Date: 15/06/2022
Pot Size: $30,000 USDC
Total HM: 5
Participants: 55
Period: 3 days
Judge: Jack the Pug
Id: 138
League: ETH
Rank: 10/55
Findings: 3
Award: $689.83
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: Picodes
Also found by: 0x1f8b, 0x52, Chom, GimelSec, IllIllI, berndartmueller, cccz, defsec, georgypetrov, hyh, kenzo, minhquanym, oyc_109, scaraven, unforgiven
50.7077 USDC - $50.71
https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L275
All funds that should have been harvested can be taken via MEV sandwich attacks because there is no slippage control.
The two swap() calls pass zero as the third argument:
File: contracts/MyStrategy.sol #1 249 uint256 balEthBptEarned = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);
File: contracts/MyStrategy.sol #2 275 harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);
When the third argument is zero, that means any amount of slippage is acceptable:
File: interfaces/balancer/IBalancerVault.sol #3 161 /** 162 * @dev Performs a swap with a single Pool. 163 * 164 * If the swap is 'given in' (the number of tokens to send to the Pool is known), it returns the amount of tokens 165 * taken from the Pool, which must be greater than or equal to `limit`. 166 * 167 * If the swap is 'given out' (the number of tokens to take from the Pool is known), it returns the amount of tokens 168 * sent to the Pool, which must be less than or equal to `limit`. 169 * 170 * Internal Balance usage and the recipient are determined by the `funds` struct. 171 * 172 * Emits a `Swap` event. 173 */ 174 function swap( 175 SingleSwap memory singleSwap, 176 FundManagement memory funds, 177 uint256 limit, 178 uint256 deadline 179 ) external payable returns (uint256);
Code inspection
Use an oracle to determine the appropriate amount of assets to expect
#0 - GalloDaSballo
2022-06-18T15:48:32Z
We use private harvests
#1 - KenzoAgada
2022-06-22T10:31:20Z
Duplicate of #155
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xFar5eer, 0xNazgul, 0xNineDec, 242, Chom, Czar102, Funen, GimelSec, Meera, Picodes, Sm4rty, Tadashi, TerrierLover, Waze, _Adam, a12jmx, asutorufos, codexploder, cryptphi, defsec, gzeon, hyh, joestakey, minhquanym, oyc_109, reassor, robee, saian, sorrynotsorry, unforgiven, zzzitron
496.9982 USDC - $497.00
| Issue | Instances | |
|---|---|---|
| 1 | require() should be used instead of assert() | 1 |
| 2 | safeApprove() is deprecated | 3 |
| 3 | Open TODOs | 2 |
| 4 | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 1 |
Total: 7 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| 1 | Using vulnerable version of OpenZeppelin | 1 |
| 2 | Missing initializer modifier on constructor | 1 |
| 3 | public functions not called by the contract should be declared external instead | 1 |
| 4 | constants should be defined rather than using magic numbers | 1 |
| 5 | Redundant cast | 1 |
| 6 | Missing event and timelock for critical parameter change | 3 |
| 7 | Use a more recent version of solidity | 1 |
| 8 | Inconsistent spacing in comments | 4 |
| 9 | Typos | 4 |
| 10 | Event is missing indexed fields | 1 |
Total: 18 instances over 10 issues
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/MyStrategy.sol #1 57: assert(IVault(_vault).token() == address(AURA));
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead
There are 3 instances of this issue:
File: contracts/MyStrategy.sol #1 65: AURA.safeApprove(address(LOCKER), type(uint256).max);
File: contracts/MyStrategy.sol #2 67: AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
File: contracts/MyStrategy.sol #3 68: WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There are 2 instances of this issue:
File: contracts/MyStrategy.sol #1 284: // TODO: Hardcode claim.account = address(this)?
File: contracts/MyStrategy.sol #2 422: // TODO: Too many SLOADs
__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/MyStrategy.sol #1 20: contract MyStrategy is BaseStrategy, ReentrancyGuardUpgradeable {
The brownie configuration file says that the project is using 3.4.0 of OpenZeppelin which has a vulnerability in initializers that call external contracts, which this code does. You're protecting against it by having the comment stating to change all state at the end, but it would be better to upgrade and use the onlyInitializing modifier
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 55: /// @dev add any extra changeable variable at end of initializer as shown
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 20: contract MyStrategy is BaseStrategy, ReentrancyGuardUpgradeable {
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 is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 56: function initialize(address _vault) public initializer {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 /// @audit 9_980 205: require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage
The type of the variable is the same as the type to which the variable is being cast
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 /// @audit address(BADGER) 430: _processExtraToken(address(BADGER), amount);
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There are 3 instances of this issue:
File: contracts/MyStrategy.sol #1 86 function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external { 87 _onlyGovernance(); 88 withdrawalSafetyCheck = newWithdrawalSafetyCheck; 89: }
File: contracts/MyStrategy.sol #2 92 function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external { 93 _onlyGovernance(); 94 processLocksOnReinvest = newProcessLocksOnReinvest; 95: }
File: contracts/MyStrategy.sol #3 98 function setBribesProcessor(IBribesProcessor newBribesProcessor) external { 99 _onlyGovernance(); 100 bribesProcessor = newBribesProcessor; 101: }
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/MyStrategy.sol #1 3: pragma solidity 0.6.12;
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 4 instances of this issue:
File: contracts/MyStrategy.sol #1 85: ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
File: contracts/MyStrategy.sol #2 91: ///@dev Should we processExpiredLocks during reinvest?
File: contracts/MyStrategy.sol #3 97: ///@dev Change the contract that handles bribes
File: contracts/MyStrategy.sol #4 183: //NOTE: This probably will always fail unless we have all tokens expired
There are 4 instances of this issue:
File: contracts/MyStrategy.sol #1 /// @audit hardcoded 105: /// @notice because token paths are hardcoded, this function is safe to be called by anyone
File: contracts/MyStrategy.sol #2 /// @audit sweeped 160: /// @notice this provides security guarantees to the depositors they can't be sweeped away
File: contracts/MyStrategy.sol #3 /// @audit compunded 218: /// after claiming rewards or swapping are auto-compunded.
File: contracts/MyStrategy.sol #4 /// @audit Hardcode 284: // TODO: Hardcode claim.account = address(this)?
indexed fieldsEach event should use three indexed fields if there are three or more fields
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 51: event RewardsCollected(address token, uint256 amount);
#0 - GalloDaSballo
2022-06-19T01:14:27Z
Disagree for this specific case, assert is fine, let the caller regret setting the wrong vault
Acknowledge, however we are using safeApprove the proper way, from 0, once
Ack
Disagree, we have it in the baseStrategy: https://github.com/Badger-Finance/badger-vaults-1.5/blob/5abb584f93d564dea039a6b6a00a0cca11dd2b42/contracts/BaseStrategy.sol#L431
Ack, not exploited hence no prob
We use initialized ser, check the bot
Disagree, we need it for the deployment
Ack
I believe it get's optimized away
Personally disagree
We like the version
k
k
Good idea
#1 - IllIllI000
2022-06-21T13:03:27Z
@GalloDaSballo Ser, there is no constructor defined in this contract therefore the default one is used, where the initializer modified is not being used
#2 - GalloDaSballo
2022-06-22T00:44:46Z
@IllIllI000 I've looked into it and had I agree with the finding, would recommend rephrasing to: Implementation contract may not be initialized. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits.
Personally our UpgradeableProxy doesn't risk being self-destructed, that said if the finding is contextualized in that way I agree with it.
In terms of the Proxy, we deploy + initialize in the same transaction via constructor(admin, logic, data) or similar, meaning initialization will not be front-run on the side of the proxy
#3 - GalloDaSballo
2022-07-13T22:35:29Z
I have changed my mind about assert we should have used require and we have changed the code
#4 - jack-the-pug
2022-07-27T13:36:22Z
The following two Low severity findings should be Non-critical given the low impact:
Agreed with the severities of the other findings.
Overall, this is an excellent QA report with top-notch formatting. I love how you put a short and clear description for each issue, with all the instances listed.
142.1175 USDC - $142.12
| Issue | Instances | |
|---|---|---|
| 1 | Repeatedly changing isClaimingBribes back to false wastes gas | 1 |
| 2 | Modifier should be skipped for sweepRewards() | 1 |
| 3 | Avoid contract existence checks by using solidity version 0.8.10 or later | 9 |
| 4 | Multiple accesses of a mapping/array should use a local variable cache | 5 |
| 5 | internal functions only called once can be inlined to save gas | 3 |
| 6 | <array>.length should not be looked up in every loop of a for-loop | 2 |
| 7 | require()/revert() strings longer than 32 bytes cost extra gas | 1 |
| 8 | Using bools for storage incurs overhead | 3 |
| 9 | Use a more recent version of solidity | 1 |
| 10 | It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied | 3 |
| 11 | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 3 |
| 12 | Splitting require() statements that use && saves gas | 1 |
| 13 | Using private rather than public for constants, saves gas | 3 |
| 14 | Use custom errors rather than revert()/require() strings to save gas | 6 |
Total: 42 instances over 14 issues
isClaimingBribes back to false wastes gasChanging the value from false to true incurs a Gsset (20000 gas), checking it in the receive() incurs a Gwarmaccess (100 gas), and changing it back incurs a Gsreset (2900 gas). Instead you can store the last value of hiddenHandDistributor and change the require() in the receive() to only allow funds when hiddenHandDistributor is msg.sender, which after the first set, would change the Gsset to a Gsreset, and avoid the later Gsreset
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 310 isClaimingBribes = true; 311 hiddenHandDistributor.claim(_claims); 312: isClaimingBribes = false;
sweepRewards()sweepRewards() calls sweepRewardToken() for each token in the array, and each call ends up checking for governance, which incurs the cost of external calls to look up the governance and or strategist addresses, each of which incur the cost of at the minimum an EXTCODESIZE (700 gas) plus the cost of an external call
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 108: _onlyGovernanceOrStrategist();
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 9 instances of this issue:
File: contracts/MyStrategy.sol /// @audit token() 57: assert(IVault(_vault).token() == address(AURA)); /// @audit balanceOf() 111: uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this)); /// @audit balanceOf() 305: beforeBalance[i] = IERC20Upgradeable(token).balanceOf(address(this)); /// @audit balanceOf() 329: uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]); /// @audit balanceOf() 362: uint256 toDeposit = IERC20Upgradeable(want).balanceOf(address(this)); /// @audit balance() 397: return IVault(vault).balance(); /// @audit getPricePerFullShare() 401: return IVault(vault).getPricePerFullShare(); /// @audit safeTransfer() 423: IERC20Upgradeable(token).safeTransfer(address(bribesProcessor), amount); /// @audit safeTransfer() 429: IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory
There are 5 instances of this issue:
File: contracts/MyStrategy.sol /// @audit earnedData[i] on line 154 154: rewards[i] = TokenAmount(earnedData[i].token, earnedData[i].amount); /// @audit harvested[0] on line 226 275: harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max); /// @audit harvested[0] on line 275 278: _reportToVault(harvested[0].amount); /// @audit harvested[0] on line 278 279: if (harvested[0].amount > 0) { /// @audit harvested[0] on line 279 280: _deposit(harvested[0].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 3 instances of this issue:
File: contracts/MyStrategy.sol #1 416 function _notifyBribesProcessor() internal { 417: bribesProcessor.notifyNewRound();
File: contracts/MyStrategy.sol #2 421: function _sendTokenToBribesProcessor(address token, uint256 amount) internal {
File: contracts/MyStrategy.sol #3 428: function _sendBadgerToTree(uint256 amount) internal {
<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 are 2 instances of this issue:
File: contracts/MyStrategy.sol #1 300: for (uint256 i = 0; i < _claims.length; i++) {
File: contracts/MyStrategy.sol #2 317: for (uint256 i = 0; i < _claims.length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 184 require( 185 balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0, 186 "You have to wait for unlock or have to manually rebalance out of it" 187: );
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/MyStrategy.sol #1 24: bool public withdrawalSafetyCheck;
File: contracts/MyStrategy.sol #2 26: bool public processLocksOnReinvest;
File: contracts/MyStrategy.sol #3 28: bool private isClaimingBribes;
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
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 is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 3: pragma solidity 0.6.12;
constant/non-immutable variables to zero than to let the default of zero be appliedNot overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings
There are 3 instances of this issue:
File: contracts/MyStrategy.sol #1 118: for(uint i = 0; i < length; i++){
File: contracts/MyStrategy.sol #2 300: for (uint256 i = 0; i < _claims.length; i++) {
File: contracts/MyStrategy.sol #3 317: for (uint256 i = 0; i < _claims.length; i++) {
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 6 gas per loop
There are 3 instances of this issue:
File: contracts/MyStrategy.sol #1 118: for(uint i = 0; i < length; i++){
File: contracts/MyStrategy.sol #2 300: for (uint256 i = 0; i < _claims.length; i++) {
File: contracts/MyStrategy.sol #3 317: for (uint256 i = 0; i < _claims.length; i++) {
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
There is 1 instance of this issue:
File: contracts/MyStrategy.sol #1 184 require( 185 balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0, 186 "You have to wait for unlock or have to manually rebalance out of it" 187: );
private rather than public for constants, saves gasIf needed, the value can be read from the verified contract source code. 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 are 3 instances of this issue:
File: contracts/MyStrategy.sol #1 45: bytes32 public constant AURABAL_BALETH_BPT_POOL_ID = 0x3dd0843a028c86e0b760b1a76929d1c5ef93a2dd000200000000000000000249;
File: contracts/MyStrategy.sol #2 46: bytes32 public constant BAL_ETH_POOL_ID = 0x5c6ee304399dbdb9c8ef030ab642b10820db8f56000200000000000000000014;
File: contracts/MyStrategy.sol #3 47: bytes32 public constant AURA_ETH_POOL_ID = 0xc29562b045d80fd77c69bec09541f5c16fe20d9d000200000000000000000251;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 6 instances of this issue:
File: contracts/MyStrategy.sol 184 require( 185 balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0, 186 "You have to wait for unlock or have to manually rebalance out of it" 187: ); 205: require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage 290: require(address(bribesProcessor) != address(0), "Bribes processor not set"); 341: require(beforeVaultBalance == _getBalance(), "Balance can't change"); 342: require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change"); 437: require(isClaimingBribes, "onlyWhileClaiming");
#0 - GalloDaSballo
2022-06-19T01:00:05Z
We could also use 1 and 2, agree
We could also just call sweeprewards separately for what it's worth
I fail to see how we would action this
These are memory values, I don't believe this to be valid
Agree
Agree
Valid but we're fine
Not particularly useful for Governance Variables
Nope
##Â 10. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied MSTORE is 3 gas ser
++i saves 5 gas compared to i++
Acknowledged
That changes the functionality of the contract
We're on 6.12 ser