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: 17/55
Findings: 3
Award: $203.40
🌟 Selected for report: 0
🚀 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/v0.0.2/contracts/MyStrategy.sol#L259
Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the protocol's loss.
MyStrategy contract has low slippage checks which can lead to being vulnerable to sandwich attacks.
A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.
In the exitPool call, you have to provide minAmountsOut, the lower limits for the tokens to receive. In short, what are the minimum amounts you would find acceptable, given the amount of BPT you are providing? A good practice would be to user queryExit in BalancerHelpers to find the current amounts of tokens you would get for your BPT, and then account for some possible slippage. Let's say that you want to allow a 1% slippage. After computing how many tokens you expect for a given amount of BPT, you'd apply a factor of 0.99 to all the amounts. These thresholds are important because it's possible for token amounts to change in the pool between the time you send your transaction and the when your transaction executes.
In the strategy contract, Its hardcoded as 2.
Code Review
IBalancerVault.ExitPoolRequest memory exitPoolRequest = IBalancerVault.ExitPoolRequest({ assets: assets, minAmountsOut: new uint256[](2), userData: abi.encode(ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, balEthBptEarned, BPT_WETH_INDEX), toInternalBalance: false });
Add minimum return amount checks based on the queryExit in BalancerHelpers.
Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.
Alternatively, check if it's feasible to send these transactions directly to a miner such that they are not visible in the public mempool.
#0 - GalloDaSballo
2022-06-19T00:37:53Z
We use private tx
#1 - GalloDaSballo
2022-06-22T00:40:51Z
dup 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
102.4813 USDC - $102.48
The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L86 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L92 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98
See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
None
Add events to all functions that change critical parameters.
The critical procedures should be two step process.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98
Code Review
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
Missing checks for zero-addresses&values may lead to infunctional protocol, if the variable addresses are updated incorrectly.
There are a few validations that could be added to the system:
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98
Code Review
Consider adding zero-address and zero value checks.
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
IERC20(token).safeApprove(address(operator), 0); IERC20(token).safeApprove(address(operator), amount);
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L67 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L68
None
Approve with a zero amount first before setting the actual amount. Consider use safeIncreaseAllowance and safeDecreaseAllowance.
PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L107
Manual Code Review
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L56
Manual Code Review
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.
Line Reference
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/brownie-config.yaml#L11
I can verify that the installed version is 3.4.0 by executing the following commands:
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/brownie-config.yaml#L11
Update the versions of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to be the latest in package.json. I also recommend double checking the versions of other dependencies as a precaution, as they may include important bug fixes.
Line Reference
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L78
Ensure that the code is well commented on both with NatSpec and inline comments for better readability and maintainability. The comments should accurately reflect what the corresponding code does.
Follow the official NatSpec comment guidelines to make the contract more readable. https://docs.soliditylang.org/en/develop/natspec-format.html
Open TODOs can point to architecture or programming issues that still need to be resolved.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L284
None
Consider resolving the TODOs before deploying.
The interface and the function should require a start index and a lenght, so that the index composition can be fetched in batches without running out of gas. If there are thousands of index components (e.g. like the Wilshire 5000 index), the function may revert.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L288
None
Define proper bounds on the for loops.
#0 - GalloDaSballo
2022-06-19T02:06:46Z
Let's just penalize this submission
50.2119 USDC - $50.21
[S]: Suggested optimation, save a decent amount of gas without compromising readability;
[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;
[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
All Contracts
Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/
Solidity 0.8.13 has some improvements too but not well tested.
Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist
All Contracts
None
Consider to upgrade pragma to at least 0.8.10.
Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Revert strings > 32 bytes are here:
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L184 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L205 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L290
Manual Review
Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L300 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L153 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L317
None
Consider applying unchecked arithmetic where overflow/underflow is not possible. Example can be seen from below.
Unchecked{i++};
Since _amount can be 0. Checking if (_amount != 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L421 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L429
All Contracts
None
Consider checking amount != 0.
Boolean is default initialized to false. There is no need assign false to variable.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L300 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L153 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L317
Code Review
bool x = false costs more gas than bool x without having any different functionality.
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
All Contracts
Solidity 0.8.14 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/
Solidity 0.8.13 has some improvements too but not well tested.
Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist
All Contracts
None
Consider to upgrade pragma to at least 0.8.13.
Using double require instead of operator && can save more gas.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L185
Code Review
Example
using &&: function check(uint x)public view{ require(x == 0 && x < 1 ); } // gas cost 21630 using double require: require(x == 0 ); require( x < 1); } } // gas cost 21622
Strict inequalities add a check of non equality which costs around 3 gas.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L230 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L279 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L323 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L387
Code Review
Use >= or <= instead of > and < when possible.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source Custom Errors in Solidity:
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Instances include:
All require Statements
Code Review
Recommended to replace revert strings with custom errors.
A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.
While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.
Contracts
None
A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L300 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L317
None
Consider to cache array length.
Solidity 0.6.5 introduced immutable as a major feature. It allows setting contract-level variables at construction time which gets stored in code rather than storage.
Consider the following generic example:
contract C { /// The owner is set during contruction time, and never changed afterwards. address public owner = msg.sender; }
In the above example, each call to the function owner() reads from storage, using a sload. After EIP-2929, this costs 2100 gas cold or 100 gas warm. However, the following snippet is more gas efficient:
contract C { /// The owner is set during contruction time, and never changed afterwards. address public immutable owner = msg.sender; }
In the above example, each storage read of the owner state variable is replaced by the instruction push32 value, where value is set during contract construction time. Unlike the last example, this costs only 3 gas.
None
Consider using immutable variable.
In some cases, having function arguments in calldata instead of memory is more optimal.
Consider the following generic example:
contract C { function add(uint[] memory arr) external returns (uint sum) { uint length = arr.length; for (uint i = 0; i < arr.length; i++) { sum += arr[i]; } } }
In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:
contract C { function add(uint[] calldata arr) external returns (uint sum) { uint length = arr.length; for (uint i = 0; i < arr.length; i++) { sum += arr[i]; } } }
In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.
Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.
In short, use calldata instead of memory if the function argument is only read.
Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L149
None
Some parameters in examples given above are later hashed. It may be beneficial for those parameters to be in memory rather than calldata.
From Pragma 0.8.0, ABI coder v2 is activated by default. The pragma abicoder v2 can be deleted from the repository. That will provide gas optimization.
1. All contracts are using abicoder v2.
None
After the 0.8.0, ABI coder v2 is activated by default. Upgrade pragma to 0.8.0 version. It is recommended to delete redundant codes.
From Solidity v0.8.0 Breaking Changes https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html