2021-06-gro-findings's People
Contributors
Watchers
Forkers
ethsecurity12021-06-gro-findings's Issues
Unnecessary check array.length == N_COINS
Handle
a_delamo
Vulnerability details
Impact
The validation require(tokenAmounts.length == N_COINS, "deposit: !length"); in the methods _stableToUsd and _stableToLp is unnecessary.
When running the following code in remix
function tt_direct(uint256[N_COINS] memory rr) external view returns(uint256){
uint256 total;
for(uint256 i; i < N_COINS; ++i) {
total += rr[i];
}
return total;
}
We get Error encoding arguments: Error: expected array value (argument=null, value="", code=INVALID_ARGUMENT, version=abi/5.1.2) when calling with an array.length < N_COINS
implicit assumptions about underlying coins
Handle
gpersoon
Vulnerability details
Impact
At several places in the code there are implicit assumptions there are exactly 3 underlying coins (e.g. N_COINS == 3)
This could be a problem if a future developer (or someone who has cloned the source), tries to update the source to more than 3 coins.
One construction "N_COINS - (i + j);" assumes that N_COINS is equal to the sum of the indexes of the coins (eg. 3 = 2 + 1 + 0). And the expression also involves 3 values/variables.
Also in calculateStableCoinExposure there is the assumption that USDC is located at position #1
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Exposure.sol#L234
function calculateStableCoinExposure(uint256[N_COINS] memory directlyExposure, uint256 curveExposure) private view returns (uint256[N_COINS] memory stableCoinExposure)
{
uint256 maker = directlyExposure[0].mul(makerUSDCExposure).div(PERCENTAGE_DECIMAL_FACTOR);
for (uint256 i = 0; i < N_COINS; i++) {
uint256 indirectExposure = curveExposure;
if (i == 1) { // implicit #1 == USDC
indirectExposure = indirectExposure.add(maker);
}
stableCoinExposure[i] = directlyExposure[i].add(indirectExposure);
}
}
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Exposure.sol#L178
function sortVaultsByDelta(
....
if (bigFirst) {
vaultIndexes[0] = maxIndex;
vaultIndexes[2] = minIndex; // assumes there are exactly 3 coins
} else {
vaultIndexes[0] = minIndex;
vaultIndexes[2] = maxIndex; // assumes there are exactly 3 coins
}
vaultIndexes[1] = N_COINS - maxIndex - minIndex; // assumes that N_COINS (3)=2 + 1 + 0
}
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/LifeGuard3Pool.sol#L310
function investSingle(
...
uint256 k = N_COINS - (i + j); // assumes that N_COINS (3)=2 + 1 + 0
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/common/Constants.sol
uint8 public constant N_COINS = 3;
Tools Used
Recommended Mitigation Steps
Make the assumption that #1==USDC explicit via a constant.
Make explicit warnings in:
- Constants.sol
- on other place where an implicit reliance exists on N_COINS = 3;
sortVaultsByDelta doesn't work as expected
Handle
gpersoon
Vulnerability details
Impact
The function sortVaultsByDelta doesn't always work as expected.
Suppose all the delta's are positive, and delta1 >= delta2 >= delta3 > 0
Then maxIndex = 0
And (delta < minDelta (==0) ) is never true, so minIndex = 0
Then (assuming bigFirst==true):
vaultIndexes[0] = maxIndex = 0
vaultIndexes[2] = minIndex = 0
vaultIndexes[1] = N_COINS - maxIndex - minIndex = 3-0-0 = 3
This is clearly not what is wanted, all vaultIndexes should be different and should be in the range [0..2]
This is due to the fact that maxDelta and minDelta are initialized with the value 0.
This all could results in withdrawing from the wrong vaults and reverts (because vaultIndexes[1] is out of range).
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Exposure.sol#L178
function sortVaultsByDelta(bool bigFirst,uint256 unifiedTotalAssets,uint256[N_COINS] calldata unifiedAssets,uint256[N_COINS] calldata targetPercents) external pure override returns (uint256[N_COINS] memory vaultIndexes) {
uint256 maxIndex;
uint256 minIndex;
int256 maxDelta;
int256 minDelta;
for (uint256 i = 0; i < N_COINS; i++) {
// Get difference between vault current assets and vault target
int256 delta = int256(
unifiedAssets[i] - unifiedTotalAssets.mul(targetPercents[i]).div(PERCENTAGE_DECIMAL_FACTOR)
);
// Establish order
if (delta > maxDelta) {
maxDelta = delta;
maxIndex = i;
} else if (delta < minDelta) {
minDelta = delta;
minIndex = i;
}
}
if (bigFirst) {
vaultIndexes[0] = maxIndex;
vaultIndexes[2] = minIndex;
} else {
vaultIndexes[0] = minIndex;
vaultIndexes[2] = maxIndex;
}
vaultIndexes[1] = N_COINS - maxIndex - minIndex;
}
Tools Used
Recommended Mitigation Steps
Initialize maxDelta and minDelta:
int256 maxDelta = -2255; // or type(int256).min when using a newer solidity version
int256 minDelta = 2255; // or type(int256).max when using a newer solidity version
Check maxIndex and minIndex are not the same
require (maxIndex != minIndex);
Using access lists can save gas due to EIP-2930 post-Berlin hard fork
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
https://eips.ethereum.org/EIPS/eip-2929
https://eips.ethereum.org/EIPS/eip-2930
https://hackmd.io/@fvictorio/gas-costs-after-berlin
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate the feasibility of using access lists to save gas due to EIPs 2929 & 2930 post-Berlin hard fork. The tooling support is WIP.
Removing redundant code can save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate code and remove logic if redundant. If this is present to handle withdrawHandler updates then ignore this recommendation.
setFeeToken doesn't check index
Handle
gpersoon
Vulnerability details
Impact
The function setFeeToken doesn't check that its parameter index is within bounds.
Whereas the comparable function setVault does perform such a check.
The risk that something goes wrong is very low though.
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L137
function setVault(uint256 index, address vault) external onlyOwner {
require(vault != address(0), "setVault: 0x");
require(index < N_COINS, "setVault: !index");
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/DepositHandler.sol#L70
function setFeeToken(uint256 index) external onlyOwner {
address token = ctrl.stablecoins()[index];
require(token != address(0), "setFeeToken: !invalid token");
Tools Used
Recommended Mitigation Steps
Add something like the following to setFeeToken
require(index < N_COINS, "setFeeToken: !index");
optimization uses extra gas
Handle
gpersoon
Vulnerability details
Impact
In the function applyFactor of GToken.sol there seems to be an attempt on gas optimization by doing:
uint256 _BASE = BASE;
However because BASE is a constant it is cheaper not to do this and just use BASE.
A minor gas optimization is possible by storing
a.mul(b) or a.mul(_BASE) in temporary variable because it is evaluated twice.
Proof of Concept
https://github.com/code-423n4/2021-06-gro/blob/main/contracts/tokens/GToken.sol
uint256 public constant BASE = DEFAULT_DECIMALS_FACTOR;
function applyFactor(uint256 a,uint256 b, bool base ) internal pure returns (uint256 resultant) {
uint256 _BASE = BASE;
uint256 diff;
if (base) {
diff = a.mul(b) % _BASE;
resultant = a.mul(b).div(_BASE);
} else {
diff = a.mul(_BASE) % b;
resultant = a.mul(_BASE).div(b);
}
if (diff >= 5E17) {
resultant = resultant.add(1);
}
}
Tools Used
Recommended Mitigation Steps
Use BASE instead of _BASE
Perhaps use the following optimization:
if (base) {
uint256 tmp=a.mul(b) ;
diff = tmp % _BASE;
resultant = tmp.div(_BASE);
} else {
uint256tmp=a.mul(_BASE) ;
diff = tmp % b;
resultant = tmp.div(b);
}
Missing input validation on _feeToken in DepositHandler constructor and setFeeToken()
Handle
0xRajeev
Vulnerability details
Impact
There is no input validation on _feeToken in constructor to check if it's referring to a valid index (only USDT=2 makes sense) in the stablecoins similar to the check in setFeeToken(), which cannot be done here because the controller variable is only set later in setDependencies(). Also, given that it is set to true and that only USDT has this capability, the constructor should really check if this value is 2 and nothing else.
Also, setFeeToken() should only allow an index of 2 for now.
Scenario: Incorrectly using a _feeToken value other than 2 will cause an unnecessary balance check because of the presumed transfer fees for that token which does not exist.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Check for _feeToken == 2 in constructor or set+check it using setFeeToken() later. Given that it is only USDT which may have fees, consider hardcoding this assumption instead of making it flexible and leaving room for error, because this is not something that applies to DAI or USDC. The entire codebase currently assumes the presence of only these three tokens in the protocol anyway.
remove a multiple inheritance from Controller
Handle
gpersoon
Vulnerability details
Impact
The contract Controller inherits from Ownable, Whitelist
Also the contract Whitelist inherits from Ownable
So Controller could also just inherit from Whitelist.
This makes the inheritance tree slightly simpler.
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/common/Whitelist.sol
import "@openzeppelin/contracts/access/Ownable.sol";
contract Whitelist is Ownable {
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L38
contract Controller is Pausable, Ownable, Whitelist, FixedStablecoins, FixedGTokens, IController {
Tools Used
Recommended Mitigation Steps
Remove the Ownable from:
contract Controller is Pausable, Ownable, Whitelist, FixedStablecoins, FixedGTokens, IController {
pragma experimental ABIEncoderV2;
Handle
gpersoon
Vulnerability details
Impact
The contracts uses solidity 0.6.0 and "pragma experimental ABIEncoderV2; "
Using an experimental construct is always risky.
In the latest solidity versions (0.8.x) the ABIEncoderV2 is no longer experimental and is present by default.
Using solidity 0.8.0 also makes the use of safemath redundant.
Proof of Concept
.\common\StructDefinitions.sol:pragma experimental ABIEncoderV2;
.\insurance\Allocation.sol:pragma experimental ABIEncoderV2;
.\insurance\Exposure.sol:pragma experimental ABIEncoderV2;
.\insurance\Insurance.sol:pragma experimental ABIEncoderV2;
.\interfaces\IAllocation.sol:pragma experimental ABIEncoderV2;
.\interfaces\IExposure.sol:pragma experimental ABIEncoderV2;
.\vaults\yearnv2\v032\IYearnV2Vault.sol:pragma experimental ABIEncoderV2;
.\vaults\yearnv2\v032\VaultAdaptorYearnV2_032.sol:pragma experimental ABIEncoderV2;
Tools Used
grep
Recommended Mitigation Steps
Think about moving to solidity 0.8.x
rug pull possible via migrate
Handle
gpersoon
Vulnerability details
Impact
The contract BaseVaultAdaptor contains the function migrate which can be used to move all the ERC20 tokens to another contract.
This can be seen as a rug pull
Even if this is well intended the project could still be called out, see for example:
https://twitter.com/RugDocIO/status/1408097542202531840
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L296
function migrate(address child) external onlyOwner {
require(child != address(0), "migrate: child == 0x");
IERC20 _token = IERC20(token);
uint256 balance = _token.balanceOf(address(this));
_token.safeTransfer(child, balance);
emit LogMigrate(address(this), child, balance);
}
Tools Used
Recommended Mitigation Steps
Remove the code or apply extra safeguards
Removing unused return values can save gas
Handle
0xRajeev
Vulnerability details
Impact
The investDelta return variable from function getVaultDeltaForDeposit() is ignored at the only call-site in DepositHandler. Removing it can save some gas.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove unused return value or add logic to use it at caller.
Uninitialized vaults/addresses will lead to reverts
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
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.
Upgrading the solc compiler to >=0.8 may save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Consider porting over code to solc >= 0.8.0 for bug fixes, security enhancements and overall optimizations for gas savings.
Rearranging order of state variable declarations to pack them will save storage slots and gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
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
Tools Used
Manual Analysis
Recommended Mitigation Steps
Moving declarations of state variables that take < 32 Bytes next to each other. E.g.: booleans next to each other or address types.
initialize maxPercentForWithdraw and maxPercentForDeposit?
Handle
gpersoon
Vulnerability details
Impact
The parameters maxPercentForWithdraw and maxPercentForDeposit are not directly initialized.
If functions, which rely on these parameters, are called, before setWhaleThresholdWithdraw/setWhaleThresholdDeposit,
then they will work in a suboptimal way.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L63
uint256 public maxPercentForWithdraw;
uint256 public maxPercentForDeposit;
function setWhaleThresholdWithdraw(uint256 _maxPercentForWithdraw) external onlyOwner {
maxPercentForWithdraw = _maxPercentForWithdraw;
emit LogNewVaultMax(false, _maxPercentForWithdraw);
}
function setWhaleThresholdDeposit(uint256 _maxPercentForDeposit) external onlyOwner {
maxPercentForDeposit = _maxPercentForDeposit;
emit LogNewVaultMax(true, _maxPercentForDeposit);
}
Tools Used
Recommended Mitigation Steps
Assign a default value to maxPercentForWithdraw and maxPercentForDeposit
or initialize the values via the constructor.
Moving logic to where required will save >=6800 gas on deposit/withdraw flows
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
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);
}
Avoid use of state variables in event emissions to save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
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
Tools Used
Manual Analysis
Recommended Mitigation Steps
Use equivalent function parameters or local variables in event emits instead of state variables.
Missing zero-address check and event parameter for _emergencyHandler
Handle
0xRajeev
Vulnerability details
Impact
Controller setWithdrawHandler() is missing a zero-address check and event parameter for _emergencyHandler which is the more critical (used rarely but in an emergency incident-response that is always time-critical ) of the two addresses.
Scenario: setWithdrawHandler() is accidentally called with _emergencyHandler = 0 address. Without a check or an event here, this error goes unnoticed (unless caught in the event from WithdrawHandler::setDependencies). There is an emergency triggered after which withdrawals are attempted via the emergencyHandler but they fail because of the zero address. The correct non-zero emergencyHandler has to be set again. Valuable time is lost and funds are lost.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Add zero-address check and event parameter for _emergencyHandler
Outdated comment at calculateWithdrawalAmountsOnPartVaults
Handle
gpersoon
Vulnerability details
Impact
The comment calculateWithdrawalAmountsOnPartVaults says it returns true of false (e.g. boolean).
However the function implementation returns:
uint256 withdrawType
Probably the comment is outdated, which can be confusing.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L329
/// @notice Calculate withdrawal amounts based on part vaults, if the sum of part vaults'
/// maxWithdrawal can meet required amount, return true and valid array,
/// otherwise return false and invalid array
...
function calculateWithdrawalAmountsOnPartVaults(uint256 amount, address[N_COINS] memory vaults)
private view returns (uint256 withdrawType, uint256[N_COINS] memory withdrawalAmounts)
{
Tools Used
Recommended Mitigation Steps
Fix the comment of function calculateWithdrawalAmountsOnPartVaults
Removing unnecessary length check for static array can save gas
Handle
0xRajeev
Vulnerability details
Impact
The tokenAmounts parameter in _stableToUsd() is a static array of N_COINS length. Solidity’s static type checking will ensure that the call’s arguments at call sites will have the same length. Both call sites of this internal function indeed pass arrays of length N_COINS. So this check can be removed to save a bit of gas.
The same pattern is present in _stableToLp().
Proof of Concept
- https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L174-L175
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L113
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L122 - https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L184-L185
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L127
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove unnecessary checks.
Unnecessary zero-address check
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove unnecessary zero-address check.
Easier way to determine strategiesLength
Handle
gpersoon
Vulnerability details
Impact
The function setStrategiesLength sets the strategiesLength for the yearn pools.
It's difficult to check this value and prevent mistakes.
However it is possible to retrieve the value from yearn by repeatedly calling withdrawalQueue(i) until you reach the value of 0
See:
https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L216
NOTE: The first time a ZERO_ADDRESS is encountered, it stops withdrawing
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L90
function setStrategiesLength(uint256 _strategiesLength) external onlyOwner {
strategiesLength = _strategiesLength;
emit LogAdaptorStrategies(_strategiesLength);
}
Tools Used
Recommended Mitigation Steps
Determine or check strategiesLength with a piece of code like:
uint len=0;
for (uint256 i = 0; i < MAX_STRATS; i++) {
if (IYearnV2Vault(vault).withdrawalQueue(i) == 0) break;
len++;
}
Caching repeatedly read state variables in local variables can save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
-
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
Tools Used
Manual Analysis
Recommended Mitigation Steps
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.
Changing function visibility from public to external/internal/private can save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
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
Tools Used
Manual Analysis
Recommended Mitigation Steps
Change function visibility from public to external/private where possible.
distributeStrategyGainLoss can be abused
Handle
gpersoon
Vulnerability details
Impact
The function distributeStrategyGainLoss does the following check to allow access to the function:
require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor");
However the expression index > 0 || index <= N_COINS + 1 is always TRUE, because the OR (||) is used (should have been AND &&)
This means the function can be executed from any originator.
uint256 index = vaultIndexes[msg.sender]; ==> index will be 0
index is smaller than N_COINS + 1, so the require will not stop access.
Also index = index - 1; will not pose an problems, because Safemath isn't used and the solidity version is lower than 8.
index will be a very 2**256-1; so the rest of function can be executed without problem,
disturbing the profit & loss calculation and thus disturbing the values of the tokens.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L355
function distributeStrategyGainLoss(uint256 gain, uint256 loss) external override {
uint256 index = vaultIndexes[msg.sender];
require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor");
..
index = index - 1;
if (index < N_COINS) {
...
} else {
if (gain > 0) {
gainUsd = ibuoy.lpToUsd(gain);
} else if (loss > 0) {
lossUsd = ibuoy.lpToUsd(loss);
}
}
ipnl.distributeStrategyGainLoss(gainUsd, lossUsd, reward);
// Check if curve spot price within tollerance, if so update them
if (ibuoy.updateRatios()) {
// If the curve ratios were successfully updated, realize system price changes
ipnl.distributePriceChange(_totalAssets());
}
}
Tools Used
Recommended Mitigation Steps
Change || to &&
Use safemath for index = index - 1;
Missing emits for declared events
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate if logic is missing and add logic+emit or remove event.
Unnecessary duplication of array
Handle
a_delamo
Vulnerability details
Impact
The methods _stableToUsd and _stableToLp in theBuoy3Pool.sol contract is duplicating the array unnecessarily and costing gas to the users.
function _stableToUsd(uint256[N_COINS] memory tokenAmounts, bool deposit)
internal
view
returns (uint256)
{
require(tokenAmounts.length == N_COINS, "deposit: !length");
uint256[N_COINS] memory _tokenAmounts;
for (uint256 i = 0; i < N_COINS; i++) {
_tokenAmounts[i] = tokenAmounts[i];
}
uint256 lpAmount = curvePool.calc_token_amount(_tokenAmounts, deposit);
return _lpToUsd(lpAmount);
}
function _stableToLp(uint256[N_COINS] memory tokenAmounts, bool deposit)
internal
view
returns (uint256)
{
require(tokenAmounts.length == N_COINS, "deposit: !length");
uint256[N_COINS] memory _tokenAmounts;
for (uint256 i = 0; i < N_COINS; i++) {
_tokenAmounts[i] = tokenAmounts[i];
}
return curvePool.calc_token_amount(_tokenAmounts, deposit);
}
initialize strategyRatioBuffer
Handle
gpersoon
Vulnerability details
Impact
In the contract BaseVaultAdaptor, the strategyRatioBuffer is not initialized (e.g. is 0)
If invest is called before setStrategyRatioBuffer then updateStrategiesDebtRatio is allways called, maybe costing unnecessary gas
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L57
uint256 public strategyRatioBuffer;
function setStrategyRatioBuffer(uint256 _strategyRatioBuffer) external onlyOwner {
strategyRatioBuffer = _strategyRatioBuffer;
emit LogNewAdaptorStrategyBuffer(_strategyRatioBuffer);
}
function invest() external override onlyWhitelist {
..
if (currentRatios[i] < targetRatios[i] && targetRatios[i].sub(currentRatios[i]) > strategyRatioBuffer) {
update = true;
break;
}
if (currentRatios[i] > targetRatios[i] && currentRatios[i].sub(targetRatios[i]) > strategyRatioBuffer) {
update = true;
break;
}
}
if (update) {
updateStrategiesDebtRatio(targetRatios);
}
}
}
Tools Used
Recommended Mitigation Steps
Perhaps initialize strategyRatioBuffer (possibly via constructor) to always have a reasonable default value.
Having only owner unpause/restart is risky
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
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.
use safemath
Handle
gpersoon
Vulnerability details
Impact
Safemath is used on several places but not everywhere. Especially on risky places like PnL and distributeStrategyGainLoss it is hardly worth the gas-savings of not using safemath.
In distributeStrategyGainLoss it does make a difference, also due to another issue.
Proof of Concept
https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L215
function handleLoss( uint256 gvtAssets, uint256 pwrdAssets, uint256 loss) private pure returns (uint256, uint256) {
uint256 maxGvtLoss = gvtAssets.sub(DEFAULT_DECIMALS_FACTOR);
if (loss > maxGvtLoss) {
...
} else {
gvtAssets = gvtAssets - loss; // won't underflow but safemath won't hurt
}
function forceDistribute() private {
uint256 total = _controller().totalAssets();
if (total > lastPwrdAssets.add(DEFAULT_DECIMALS_FACTOR)) {
lastGvtAssets = total - lastPwrdAssets; // won't underflow but safemath won't hurt
} else {
...
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L355
function distributeStrategyGainLoss(uint256 gain, uint256 loss) external override {
uint256 index = vaultIndexes[msg.sender];
require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor"); // always true, see separate issue
..
index = index - 1; // can underflow
Tools Used
Recommended Mitigation Steps
Apply safemath or move to Solidity 0.8.x
Removing unnecessary check can save gas in withdraw flow
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove unnecessary check.
setUnderlyingTokenPercent should check percentages
Handle
gpersoon
Vulnerability details
Impact
The function setUnderlyingTokenPercent doesn't check that the sum of all the percentages is 100%.
This way the percentages could be accidentally set up the wrong way, with unpredictable results.
Note: the function can only be called by controller or the owner so the likely hood of mistakes is pretty low.
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L100
function setUnderlyingTokenPercent(uint256 coinIndex, uint256 percent) external override onlyValidIndex(coinIndex) {
require(msg.sender == controller || msg.sender == owner(), "setUnderlyingTokenPercent: !authorized");
underlyingTokensPercents[coinIndex] = percent;
emit LogNewTargetAllocation(coinIndex, percent);
}
Tools Used
Recommended Mitigation Steps
Change setUnderlyingTokenPercent to set the percentages for all the coins at the same time.
And check that the sum of the percentages is 100%
calcProtocolExposureDelta could use a break
Handle
gpersoon
Vulnerability details
Impact
calcProtocolExposureDelta should probably stop executing once it has found the first occurrence where exposure > threshold.
(as is also indicated in the comment).
The current code also works (due to the check protocolExposedDeltaUsd == 0), however inserting a break statement at the end of the "if" is more logical and saves a bit of gas.
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Allocation.sol#L286
/// By defenition, only one protocol can exceed exposure in the current setup.
...
function calcProtocolExposureDelta(uint256[] memory protocolExposure, SystemState memory sysState) private pure
returns (uint256 protocolExposedDeltaUsd, uint256 protocolExposedIndex)
{
for (uint256 i = 0; i < protocolExposure.length; i++) {
// If the exposure is greater than the rebalance threshold...
if (protocolExposedDeltaUsd == 0 && protocolExposure[i] > sysState.rebalanceThreshold) {
// ...Calculate the delta between exposure and target
uint256 target = sysState.rebalanceThreshold.sub(sysState.targetBuffer);
protocolExposedDeltaUsd = protocolExposure[i].sub(target).mul(sysState.totalCurrentAssetsUsd).div(
PERCENTAGE_DECIMAL_FACTOR
);
protocolExposedIndex = i;
// probably put a break here
}
}
}
Tools Used
Recommended Mitigation Steps
Add a break statement at the end of the if
lastRatio of Buoy3Pool not initialized
Handle
gpersoon
Vulnerability details
Impact
The values of lastRatio in the contract Buoy3Pool are not initialized (thus they have a value of 0).
If safetyCheck() would be called before the first time _updateRatios is called, then safetyCheck() would give unexpected results.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L25
contract Buoy3Pool is FixedStablecoins, Controllable, IBuoy, IChainPrice {
...
mapping(uint256 => uint256) lastRatio;
function safetyCheck() external view override returns (bool) {
for (uint256 i = 1; i < N_COINS; i++) {
uint256 _ratio = curvePool.get_dy(int128(0), int128(i), getDecimal(0));
_ratio = abs(int256(_ratio - lastRatio[i]));
if (_ratio.mul(PERCENTAGE_DECIMAL_FACTOR).div(CURVE_RATIO_DECIMALS_FACTOR) > BASIS_POINTS) {
return false;
}
}
return true;
}
function _updateRatios(uint256 tolerance) private returns (bool) {
...
for (uint256 i = 1; i < N_COINS; i++) {
lastRatio[i] = newRatios[i];
Tools Used
Recommended Mitigation Steps
Double check if this situation can occur.
Perhaps call _updateRatios as soon as possible.
Or check in safetyCheck that the lastRatio values are initialized
name of return variable not logical
Handle
gpersoon
Vulnerability details
Impact
The return variable of getUserAssets has the name deductUsd.
This doesn't seem like a logical name for the result of the function.
Although this isn't a real problem it makes the code more difficult to understand.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L430
function getUserAssets(bool pwrd, address account) external view override returns (uint256 deductUsd) {
IToken gt = gTokens(pwrd);
deductUsd = gt.getAssets(account);
require(deductUsd > 0, "!minAmount");
}
Tools Used
Recommended Mitigation Steps
Use a more appropriate variable name like:
uint256 assets
redundant check of array length
Handle
gpersoon
Vulnerability details
Impact
The function _stableToUsd and _stableToLp check that the size of the input array is right.
However because that parameter definition also contains the length (e.g. [N_COINS] ), it is already checked by solidity.
So checking it again is not necessary.
Note: if this would be necessary than it should also be done at the other functions that have an input parameter with [N_COINS], see at Proof of concept.
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L174
function _stableToUsd(uint256[N_COINS] memory tokenAmounts, bool deposit) internal view returns (uint256) {
require(tokenAmounts.length == N_COINS, "deposit: !length");
...
function _stableToLp(uint256[N_COINS] memory tokenAmounts, bool deposit) internal view returns (uint256) {
require(tokenAmounts.length == N_COINS, "deposit: !length");
..
Other functions with a [N_COINS] parameter:
.\Controller.sol: function distributeCurveAssets(uint256 amount, uint256[N_COINS] memory delta) external onlyWhitelist {
.\DepositHandler.sol: function _invest(uint256[N_COINS] memory _inAmounts, uint256 roughUsd) internal returns (uint256 dollarAmount) {
.\DepositHandler.sol: function roughUsd(uint256[N_COINS] memory inAmounts) private view returns (uint256 usdAmount) {
.\WithdrawHandler.sol: function withdrawAllBalanced(bool pwrd, uint256[N_COINS] calldata minAmounts) external override {
.\insurance\Exposure.sol: function getUnifiedAssets(address[N_COINS] calldata vaults)
.\insurance\Exposure.sol: function calculateStableCoinExposure(uint256[N_COINS] memory directlyExposure, uint256 curveExposure)
.\insurance\Insurance.sol: function calculateWithdrawalAmountsOnPartVaults(uint256 amount, address[N_COINS] memory vaults)
.\insurance\Insurance.sol: function calculateWithdrawalAmountsOnAllVaults(uint256 amount, address[N_COINS] memory vaults)
.\pools\LifeGuard3Pool.sol: function distributeCurveVault(uint256 amount, uint256[N_COINS] memory delta)
.\pools\LifeGuard3Pool.sol: function invest(uint256 depositAmount, uint256[N_COINS] calldata delta)
.\pools\LifeGuard3Pool.sol: function _withdrawUnbalanced(uint256 inAmount, uint256[N_COINS] memory delta) private {
.\pools\oracle\Buoy3Pool.sol: function stableToUsd(uint256[N_COINS] calldata inAmounts, bool deposit) external view override returns (uint256) {
.\pools\oracle\Buoy3Pool.sol: function stableToLp(uint256[N_COINS] calldata tokenAmounts, bool deposit) external view override returns (uint256) {
Tools Used
Recommended Mitigation Steps
Remove :
require(tokenAmounts.length == N_COINS, "deposit: !length");
Unnecessary update of amount
Handle
gpersoon
Vulnerability details
Impact
In several functions of BaseVaultAdaptor a value is stored in the variable amount at the end of the function.
However this variable is never used afterwards so the storage is unnecessary and just uses gas.
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L165
function withdraw(uint256 amount) external override {
..
if (!_withdrawFromAdapter(amount, msg.sender)) {
amount = _withdraw(calculateShare(amount), msg.sender);
function withdraw(uint256 amount, address recipient) external override {
...
if (!_withdrawFromAdapter(amount, recipient)) {
amount = _withdraw(calculateShare(amount), recipient);
function withdrawToAdapter(uint256 amount) external onlyOwner {
amount = _withdraw(calculateShare(amount), address(this));
}
function withdrawByStrategyOrder(
..
if (!_withdrawFromAdapter(amount, recipient)) {
amount = _withdrawByStrategyOrder(calculateShare(amount), recipient, reversed);
function withdrawByStrategyIndex(
...
if (!_withdrawFromAdapter(amount, recipient)) {
amount = _withdrawByStrategyIndex(calculateShare(amount), recipient, strategyIndex);
Tools Used
Recommended Mitigation Steps
Replace
amount = _withdraw***(...);
with
_withdraw***(...);
Simplifying logic will save at least 4200-11,500 gas in deposit flow
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate removing it completely or hardcoding logic only for USDT index=2 to save gas.
Removing unnecessary lpToken.balanceOf can save 4700+ gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove the call on L204 and just get the balance on L206 without any subtraction.
require comments don't all follow convention
Handle
gpersoon
Vulnerability details
Impact
The require comments don't always follow the same convention.
The convention seems to be "functionname: !error"
However the following situations also occur:
- Function prefixed by!
- No string
- No function name
The revert reasons will be easier to understand it they all follow the convention.
Proof of Concept
Function prefixed by!:
.\WithdrawHandler.sol: require(tokenAmount >= minAmount, "!withdrawSingle: !minAmount");
.\WithdrawHandler.sol: require(tokenAmounts[i] >= minAmounts[i], "!withdrawBalanced: !minAmount");
.\WithdrawHandler.sol: require(minAmount <= amount, "!prepareForWithdrawalSingle: !minAmount");
.\pools\LifeGuard3Pool.sol: require(msg.sender == depositHandler, "!investSingle: !depositHandler");
No string:
.\insurance\Insurance.sol: require(buoy.safetyCheck());
.\pools\LifeGuard3Pool.sol: require(balance >= minAmount);
.\vaults\BaseVaultAdaptor.sol: require(msg.sender == vault);
.\vaults\BaseVaultAdaptor.sol: require(reserve <= PERCENTAGE_DECIMAL_FACTOR);
No function name:
.\Controller.sol: require(_percent > 0, "_whaleLimit is 0");
.\Controller.sol: require(msg.sender == depositHandler, "!depositHandler");
.\Controller.sol: require(sender == tx.origin, "EOA only");
.\Controller.sol: require(IBuoy(buoy).safetyCheck(), "!buoy.safetyCheck");
.\Controller.sol: require(coin < N_COINS, "invalid coin");
.\Controller.sol: require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor");
.\Controller.sol: require(deductUsd > 0, "!minAmount");
.\DepositHandler.sol: require(minAmount > 0, "minAmount is 0");
.\DepositHandler.sol: require(buoy.safetyCheck(), "!safetyCheck");
.\DepositHandler.sol: require(dollarAmount >= buoy.lpToUsd(minAmount), "!minAmount");
.\WithdrawHandler.sol: require(lpAmount > 0, "!minAmount");
.\WithdrawHandler.sol: require(lpAmount > 0, "!minAmount");
.\WithdrawHandler.sol: require(buoy.safetyCheck(), "!safetyCheck");
.\common\Controllable.sol: require(controller != address(0), "Controller not set");
.\common\Controllable.sol: require(controller != address(0), "Controller not set");
.\common\Whitelist.sol: require(whitelist[msg.sender], "only whitelist");
.\insurance\Exposure.sol: require(totalAssets > withdrawUsd, "totalAssets < withdrawalUsd");
.\insurance\Insurance.sol: require(index >= 0 && index < N_COINS, "Invalid index value.");
.\insurance\Insurance.sol: require(_allocation != address(0), "Zero address provided");
.\insurance\Insurance.sol: require(_exposure != address(0), "Zero address provided");
.\insurance\Insurance.sol: require(curveVaultUsd > leftUsd, "no enough system assets");
.\insurance\Insurance.sol: require(withdrawAmount < state.totalCurrentAssetsUsd, "Withdrawal exceeds system assets");
.\pnl\PnL.sol: require(msg.sender == controller, "!Controller");
.\pnl\PnL.sol: require(msg.sender == controller, "!Controller");
.\tokens\NonRebasingGToken.sol: require(amount > 0, "Amount is zero.");
.\tokens\NonRebasingGToken.sol: require(amount > 0, "Amount is zero.");
.\tokens\RebasingGToken.sol: require(amount > 0, "Amount is zero.");
.\tokens\RebasingGToken.sol: require(amount > 0, "Amount is zero.");
.\vaults\BaseVaultAdaptor.sol: require(index < strategiesLength, "invalid index");
.\vaults\BaseVaultAdaptor.sol: require(index < strategiesLength, "invalid index");
.\vaults\yearnv2\v032\VaultAdaptorYearnV2_032.sol: require(ratioTotal <= 10**4, "The total of ratios is more than 10000");
Tools Used
grep
Recommended Mitigation Steps
Apply the same require convention everywhere
BASIS_POINTS naming convention
Handle
gpersoon
Vulnerability details
Impact
The variable BASIS_POINTS in Buoy3Pool.sol is written in capitals, which is the naming convention for constants.
However BASIS_POINTS isn't a constant, because it is updated in setBasisPointsLmit
This is confusing when reading the code.
Proof of Concept
https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L30
uint256 public BASIS_POINTS = 20;
function setBasisPointsLmit(uint256 newLimit) external onlyOwner {
uint256 oldLimit = BASIS_POINTS;
BASIS_POINTS = newLimit;
emit LogNewBasisPointLimit(oldLimit, newLimit);
}
Tools Used
Recommended Mitigation Steps
Change BASIS_POINTS to something like:
basisPoints
Single-step process for critical ownership transfer is risky
Handle
0xRajeev
Vulnerability details
Impact
The Controller contract is arguably the most critical contract in the project for access control management (it has 17 onlyOwner functions). Given that it is derived from Ownable, the ownership management of this contract (also Whitelist and Controllable) defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes.
The same applies to the changing of controller’s address in contracts deriving from Controllable using setController().
Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.
Proof of Concept
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
And many other onlyOwner functions such as setController():
Tools Used
Manual Analysis
Recommended Mitigation Steps
Override the inherited methods to null functions and use separate functions for a two-step address change: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.
Unnecessary copying of memory variables can save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
- Use tokenAmounts directly instead of copying to _tokenAmounts
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L174-L180
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L184-L190 - Use _vaultIndexes directly instead of copying to vaultIndexes
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L160-L171 - Use withdrawalAmounts instead of copying to _withdrawalAmounts
** https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L316-L320
Tools Used
Manual Analysis
Recommended Mitigation Steps
Avoid redundant copying to save a bit of gas and improve readability.
Simpler logic can save gas
Handle
0xRajeev
Vulnerability details
Impact
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.
Proof of Concept
Tools Used
Manual Analysis
Recommended Mitigation Steps
Replace L317 to L323 with:
uint256 inBalance = inAmounts[N_COINS - (i + j)];
if (inBalance > 0) {
_exchange(inBalance, int128(k), int128(i));
}
hardcoded values
Handle
gpersoon
Vulnerability details
Impact
There are several hardcodes values that could very well be replaced with constants.
For example:
- 10**18
- 5E17
- 10000
- 10**4
- 3 (N_COINS)
This will make the code more readable and easier to maintain
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/DepositHandler.sol#L206
function roughUsd(uint256[N_COINS] memory inAmounts) private view returns (uint256 usdAmount) {
..
usdAmount = usdAmount.add(inAmounts[i].mul(10**18).div(getDecimal(i)));
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/tokens/GToken.sol#L24
abstract contract GToken is GERC20, Constants, Whitelist, IToken {
uint256 public constant BASE = DEFAULT_DECIMALS_FACTOR;
function applyFactor(
....
if (diff >= 5E17) {
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/yearnv2/v032/VaultAdaptorYearnV2_032.sol#L107
function updateStrategiesDebtRatio(uint256[] memory ratios) internal override {
..
require(ratioTotal <= 10**4, "The total of ratios is more than 10000");
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L317
function emergency(uint256 coin) external onlyWhitelist {
...
percent = 10000;
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L144
function getVaultDeltaForDeposit(uint256 amount)
....
investDelta[vaultIndexes[0]] = 10000;
.\common\StructDefinitions.sol: uint256[3] vaultCurrentAssets;
.\common\StructDefinitions.sol: uint256[3] vaultCurrentAssetsUsd;
.\common\StructDefinitions.sol: uint256[3] stablePercents;
.\common\StructDefinitions.sol: uint256[3] stablecoinExposure;
.\common\StructDefinitions.sol: uint256[3] protocolWithdrawalUsd;
.\common\StructDefinitions.sol: uint256[3] swapInAmounts;
.\common\StructDefinitions.sol: uint256[3] swapInAmountsUsd;
.\common\StructDefinitions.sol: uint256[3] swapOutPercents;
.\common\StructDefinitions.sol: uint256[3] vaultsTargetUsd;
.\interfaces\IBuoy.sol: function stableToUsd(uint256[3] calldata inAmount, bool deposit) external view returns (uint256);
.\interfaces\IBuoy.sol: function stableToLp(uint256[3] calldata inAmount, bool deposit) external view returns (uint256);
.\interfaces\IController.sol: function stablecoins() external view returns (address[3] memory);
.\interfaces\IController.sol: function vaults() external view returns (address[3] memory);
.\interfaces\ICurve.sol: function calc_token_amount(uint256[3] calldata inAmounts, bool deposit) external view returns (uint256);
.\interfaces\ICurve.sol: function add_liquidity(uint256[3] calldata uamounts, uint256 min_mint_amount) external;
.\interfaces\ICurve.sol: function remove_liquidity(uint256 amount, uint256[3] calldata min_uamounts) external;
.\interfaces\ICurve.sol: function remove_liquidity_imbalance(uint256[3] calldata amounts, uint256 max_burn_amount) external;
.\interfaces\IDepositHandler.sol: uint256[3] calldata inAmounts,
.\interfaces\IDepositHandler.sol: uint256[3] calldata inAmounts,
.\interfaces\IExposure.sol: function getUnifiedAssets(address[3] calldata vaults)
.\interfaces\IExposure.sol: returns (uint256 unifiedTotalAssets, uint256[3] memory unifiedAssets);
.\interfaces\IExposure.sol: uint256[3] calldata unifiedAssets,
.\interfaces\IExposure.sol: uint256[3] calldata targetPercents
.\interfaces\IExposure.sol: ) external pure returns (uint256[3] memory vaultIndexes);
.\interfaces\IExposure.sol: uint256[3] calldata targets,
.\interfaces\IExposure.sol: address[3] calldata vaults,
.\interfaces\IExposure.sol: ) external view returns (uint256[3] memory);
.\interfaces\IInsurance.sol: function calculateDepositDeltasOnAllVaults() external view returns (uint256[3] memory);
.\interfaces\IInsurance.sol: function getDelta(uint256 withdrawUsd) external view returns (uint256[3] memory delta);
.\interfaces\IInsurance.sol: uint256[3] memory,
.\interfaces\IInsurance.sol: uint256[3] memory,
.\interfaces\IInsurance.sol: function sortVaultsByDelta(bool bigFirst) external view returns (uint256[3] memory vaultIndexes);
.\interfaces\ILifeGuard.sol: function getAssets() external view returns (uint256[3] memory);
.\interfaces\ILifeGuard.sol: function distributeCurveVault(uint256 amount, uint256[3] memory delta) external returns (uint256[3] memory);
.\interfaces\ILifeGuard.sol: function invest(uint256 whaleDepositAmount, uint256[3] calldata delta) external returns (uint256 dollarAmount);
.\interfaces\ILifeGuard.sol: uint256[3] calldata inAmounts,
.\interfaces\IWithdrawHandler.sol: uint256[3] calldata minAmounts
.\interfaces\IWithdrawHandler.sol: function withdrawAllBalanced(bool pwrd, uint256[3] calldata minAmounts) external;
.\pools\oracle\Buoy3Pool.sol: function getTokenRatios(uint256 i) private view returns (uint256[3] memory _ratios) {
.\pools\oracle\Buoy3Pool.sol: uint256[3] memory _prices;
.\pools\oracle\Buoy3Pool.sol: for (uint256 j = 0; j < 3; j++) {
Tools Used
Recommended Mitigation Steps
Do the following replacements
- 10**18 ==> DEFAULT_DECIMALS_FACTOR
- 5E17 ==> DEFAULT_DECIMALS_FACTOR /2 or BASE/2
- 10000 ==> PERCENTAGE_DECIMAL_FACTOR
- 10**4 ==> PERCENTAGE_DECIMAL_FACTOR
- 3 ==> N_COINS
emergencyHandler not checked & not emitted
Handle
gpersoon
Vulnerability details
Impact
The function setWithdrawHandler allows the setting of withdrawHandler and emergencyHandler.
However emergencyHandler isn't checked for 0 (like the withdrawHandler )
The value of the emergencyHandler is also not emitted (like the withdrawHandler )
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L105
function setWithdrawHandler(address _withdrawHandler, address _emergencyHandler) external onlyOwner {
require(_withdrawHandler != address(0), "setWithdrawHandler: 0x");
withdrawHandler = _withdrawHandler;
emergencyHandler = _emergencyHandler;
emit LogNewWithdrawHandler(_withdrawHandler);
}
Tools Used
Recommended Mitigation Steps
Add something like:
require(_emergencyHandler!= address(0), "setEmergencyHandler: 0x");
event LogNewEmergencyHandler(address tokens);
emit LogNewEmergencyHandler(_emergencyHandler);
implicit underflows
Handle
gpersoon
Vulnerability details
Impact
There are a few underflows that are converted via a typecast afterwards to the expected value. If solidity 0.8.x would be used, then the code would revert.
int256(a-b) where a and b are uint, For example if a=1 and b=2 then the intermediate result would be uint(-1) == 2256-1
int256(-x) where x is a uint. For example if x=1 then the intermediate result would be uint(-1) == 2256-1
Its better not to have underflows by using the appropriate typecasts.
This is especially relevant when moving to solidity 0.8.x
Proof of Concept
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Exposure.sol#L178
function sortVaultsByDelta(..)
..
for (uint256 i = 0; i < N_COINS; i++) {
// Get difference between vault current assets and vault target
int256 delta = int256(unifiedAssets[i] - unifiedTotalAssets.mul(targetPercents[i]).div(PERCENTAGE_DECIMAL_FACTOR)); // underflow in intermediate result
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L112
function decreaseGTokenLastAmount(bool pwrd, uint256 dollarAmount, uint256 bonus)...
..
emit LogNewGtokenChange(pwrd, int256(-dollarAmount)); // underflow in intermediate result
// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L87
function safetyCheck() external view override returns (bool) {
...
_ratio = abs(int256(_ratio - lastRatio[i])); // underflow in intermediate result
Tools Used
Recommended Mitigation Steps
replace int256(a-b) with int256(a)-int256(b)
replace int256(-x) with -int256(x)
Removing unnecessary initializations can save gas
Handle
0xRajeev
Vulnerability details
Impact
Declarations of state variables need not unnecessarily initialize them to the type’s default values. This will save a bit of gas.
Proof of Concept
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
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove unnecessary initializations
distributePriceChange might revert
Handle
gpersoon
Vulnerability details
Impact
The function distributePriceChange includes the following statement:
lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(totalAssets));
If you look at this:
lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(gvtAssets.add(pwrdAssets)));
lastGvtAssets = gvtAssets + currentTotalAssets- gvtAssets -pwrdAssets
lastGvtAssets = currentTotalAssets - pwrdAssets (with checking of underflows via safemath)
Then you can see it will revert if currentTotalAssets < pwrdAssets
In that situation the administration will not be updated appropriately
Proof of Concept
//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L282
function distributePriceChange(uint256 currentTotalAssets) external override {
require(msg.sender == controller, "!Controller");
uint256 gvtAssets = lastGvtAssets;
uint256 pwrdAssets = lastPwrdAssets;
uint256 totalAssets = gvtAssets.add(pwrdAssets);
if (currentTotalAssets > totalAssets) {
lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(totalAssets));
} else if (currentTotalAssets < totalAssets) {
(lastGvtAssets, lastPwrdAssets) = handleLoss(gvtAssets, pwrdAssets, totalAssets.sub(currentTotalAssets));
}
int256 priceChange = int256(currentTotalAssets) - int256(totalAssets);
...
}
Tools Used
Recommended Mitigation Steps
Detect the situation where currentTotalAssets < pwrdAssets and take appropriate recovery actions.
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.