Platform: Code4rena
Start Date: 06/01/2023
Pot Size: $210,500 USDC
Total HM: 27
Participants: 73
Period: 14 days
Judge: 0xean
Total Solo HM: 18
Id: 203
League: ETH
Rank: 73/73
Findings: 1
Award: $72.44
π Selected for report: 0
π Solo Findings: 0
π Selected for report: IllIllI
Also found by: 0xA5DF, 0xSmartContract, 0xhacksmithh, AkshaySrivastav, Awesome, Aymen0909, Bauer, Bnke0x0, Breeje, Budaghyan, Cyfrin, Madalad, NoamYakov, RHaO-sec, Rageur, RaymondFam, ReyAdmirado, Rolezn, SAAJ, SaharDevep, Sathish9098, __141345__, amshirif, arialblack14, c3phas, carlitox477, chaduke, delfin454000, descharre, nadin, oyc_109, pavankv, saneryee, shark
72.4433 USDC - $72.44
Use calldata instead of memory for function parameters saves gas if the function argument is only read.
Use calldata instead of memory.
protocol/contracts/libraries/String.sol#L11 | function toLower(string memory str) internal pure returns (string memory) { |
protocol/contracts/libraries/test/StringCallerMock.sol#L9 | function toLower(string memory str) external pure returns (string memory){ |
++i/i++
should be unchecked{++i}
/unchecked{i++}
when it is not possible for them to overflow, for example when used in for
and while
loopsIn Solidity 0.8+, thereβs a default overflow check on unsigned integers. Itβs possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline. Example for loop:
for (uint i = 0; i < length; i++) { // do something that doesn't change the value of i }
In this example, the for loop post condition, i.e., i++
involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1
. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2
. This means that the i++
in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.
Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks. You should manually do this by:
for (uint i = 0; i < length; i = unchecked_inc(i)) { // do something that doesn't change the value of i } function unchecked_inc(uint i) returns (uint) { unchecked { return i + 1; } }
Or just:
for (uint i = 0; i < length;) { // do something that doesn't change the value of i unchecked { i++; } }
Note that itβs important that the call to unchecked_inc
is inlined. This is only possible for solidity versions starting from 0.8.2
.
Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant! (This is only relevant if you are using the default solidity checked arithmetic.)
Use the unchecked
keyword
protocol/contracts/libraries/String.sol#L14 | for (uint256 i = 0; i < bStr.length; i++) { |
protocol/contracts/p1/BasketHandler.sol#L653 | for (uint256 i = 0; i < erc20s.length; i++) { |
protocol/contracts/p1/Distributor.sol#L133 | for (uint256 i = 0; i < numTransfers; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L277 | for (uint256 i = 0; i < _minBuyAmounts.length; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L288 | for (uint256 i = 0; i < _minBuyAmounts.length; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L316 | for (uint256 i = 0; i < _sellOrders.length; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L344 | for (uint256 i = 0; i < iterationSteps; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L513 | for (uint256 i = 0; i < orders.length; i++) { |
protocol/contracts/plugins/mocks/EasyAuction.sol#L524 | for (uint256 i = 0; i < orders.length; i++) { |
You can save about 6 gas per instance if using assembly to check for address(0)
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~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.
Where appropriate, you can combine multiple address mappings into a single mapping of an address to a struct.
protocol/contracts/p1/StRSR.sol#L68 | mapping(uint256 => mapping(address => mapping(address => uint256))) private _allowances; |
protocol/contracts/plugins/aave/ERC20.sol#L40 | mapping(address => mapping(address => uint256)) private _allowances; |
protocol/contracts/plugins/mocks/WETH.sol#L31 | mapping(address => mapping(address => uint256)) public allowance; |
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct(src)
Use storage for struct/array
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.
If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified
solidity if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}
Empty blocks should be removed or emit something (The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.
require
statements instead of operator &&
to save gas.Usage of double require will save you around 10 gas with the optimizer enabled. See 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. Example:
contract Requires { uint256 public gas; function check1(uint x) public { gas = gasleft(); require(x == 0 && x < 1 ); // gas cost 22156 gas -= gasleft(); } function check2(uint x) public { gas = gasleft(); require(x == 0); // gas cost 22148 require(x < 1); gas -= gasleft(); } }
Consider changing one require()
statement to two require()
to save gas
abi.encode()
is less efficient than abi.encodePacked()
abi.encode
will apply ABI encoding rules. Therefore all elementary types are padded to 32 bytes and dynamic arrays include their length. Therefore it is possible to also decode this data again (with abi.decode
) when the type are known.
abi.encodePacked
will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode.
For the input of the keccak
method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts.
For example:
abi.encodePacked(address(0x0000000000000000000000000000000000000001), uint(0))
encodes to the same as abi.encodePacked(uint(0x0000000000000000000000000000000000000001000000000000000000000000), address(0))
and abi.encodePacked(uint[](1,2), uint[](3))
encodes to the same as abi.encodePacked(uint[](1), uint[](2,3))
Therefore these examples will also generate the same hashes even so they are different inputs.
On the other hand you require less memory and therefore in most cases abi.encodePacked uses less gas than abi.encode.
Use abi.encodePacked()
where possible to save gas
protocol/contracts/p1/StRSR.sol#L778 | abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline) |
protocol/contracts/p1/StRSRVotes.sol#L128 | _hashTypedDataV4(keccak256(abi.encode(_DELEGATE_TYPEHASH, delegatee, nonce, expiry))), |
protocol/contracts/plugins/aave/StaticATokenLM.sol#L151 | abi.encode(PERMIT_TYPEHASH, owner, spender, value, currentValidNonce, deadline) |
protocol/contracts/plugins/aave/StaticATokenLM.sol#L179 | abi.encode( |
protocol/contracts/plugins/aave/StaticATokenLM.sol#L219 | abi.encode( |
protocol/contracts/plugins/aave/StaticATokenLM.sol#L269 | abi.encode( |
Not inlining costs 20 to 40 gas because of two extra JUMP
instructions and additional stack operations needed for function calls.
Check this out: link
Note: To sum up, when there is an internal function that is only called once, there is no point for that function to exist.
TODO - CHECK IF THIS IS REALLY THE CASE!!!!
protocol/contracts/p1/BasketHandler.sol#L68 | function empty(Basket storage self) internal { |
protocol/contracts/plugins/aave/ERC20.sol#L324 | function setupDecimals(uint8 decimals) internal { |
protocol/contracts/plugins/mocks/vendor/EasyAuction.sol#L243 | function initializeEmptyList(Data storage self) internal { |
#0 - c4-judge
2023-01-24T23:19:48Z
0xean marked the issue as grade-b