Alchemix contest - 0xkatana's results

A protocol for self-repaying loans with no liquidation risk.

General Information

Platform: Code4rena

Start Date: 05/05/2022

Pot Size: $125,000 DAI

Total HM: 17

Participants: 62

Period: 14 days

Judge: leastwood

Total Solo HM: 15

Id: 120

League: ETH

Alchemix

Findings Distribution

Researcher Performance

Rank: 20/62

Findings: 2

Award: $379.61

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Alchemix

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xkatana, 0xsomeone, AuditsAreUS, BouSalman, BowTiedWardens, Cityscape, Funen, GimelSec, Hawkeye, JC, MaratCerby, MiloTruck, Picodes, Ruhum, TerrierLover, WatchPug, Waze, bobirichman, catchup, cccz, cryptphi, csanuragjain, delfin454000, ellahi, fatherOfBlocks, hake, horsefacts, hyh, jayjonah8, joestakey, kebabsec, kenta, mics, oyc_109, robee, samruna, shenwilly, sikorico, simon135, throttle, tintin

Awards

178.3839 DAI - $178.38

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

[L-01] safeApprove is deprecated

The _setupRole function is deprecated according to the Open Zeppelin comment Deprecated. This function has issues similar to the ones found in {IERC20-approve}, and its usage is discouraged.

This Open Zeppelin comment indicates it is deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L39-L43

Recommended Mitigation Steps

Replace safeApprove with {safeIncreaseAllowance} or {safeDecreaseAllowance} instead

[L-02] Deprecated `_setupRole` function used

The _setupRole function is deprecated according to the Open Zeppelin comment NOTE: This function is deprecated in favor of {_grantRole}

Use the recommended _grantRole function instead.

This Open Zeppelin comment indicates it is deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L195

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, 0xsomeone, AlleyCat, BowTiedWardens, Cityscape, Fitraldys, Funen, GimelSec, Hawkeye, JC, MaratCerby, MiloTruck, Randyyy, TerrierLover, Tomio, UnusualTurtle, WatchPug, Waze, _Adam, augustg, bobirichman, catchup, csanuragjain, ellahi, fatherOfBlocks, hake, hansfriese, horsefacts, ignacio, joestakey, kenta, mics, oyc_109, robee, samruna, sashik_eth, sikorico, simon135, throttle

Awards

201.2269 DAI - $201.23

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

[G-01] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/StakingPools.sol#L106 https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/StakingPools.sol#L124 https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/StakingPools.sol#L131 https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/StakingPools.sol#L160 https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/StakingPools.sol#L183

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-02] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-03] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-04] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-05] For loop incrementing can be unsafe

For loops that use i++ do not need to use safemath for this operation because the loop would run out of gas long before this point. Making this addition operation unsafe using unchecked saves gas.

Sample code to make the for loop increment unsafe

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;
    }
}

Idea borrowed from https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#the-increment-in-for-loop-post-condition-can-be-made-unchecked

Recommended Mitigation Steps

Make the increment in for loops unsafe to save gas

[G-06] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Recommended Mitigation Steps

Cache the array length before the for loop

[G-07] Non-public variables save gas

Many constant variables are public, but changing the visibility of this variable to private or internal can save gas.

Recommended Mitigation Steps

Declare some public variables as private or internal to save gas

[G-08] Bitshift for divide by 2

When multiply or dividing by a power of two, it is cheaper to bitshift than to use standard math operations.

There is a divide by 2 operation on this line https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/ThreePoolAssetManager.sol#L355

Recommended Mitigation Steps

Bitshift right by one bit instead of dividing by 2 to save gas

[G-09] Add payable to functions that won't receive ETH

Identifying a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas. This is especially relevant because withdrawAll functions exist to withdraw any ETH accidentally sent.

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-10] Replace `_setupRole` call with `_grantRole`

Impact

The _setupRole function calls _grantRole, so it would save gas to call _grantRole directly to remove the function call

This Open Zeppelin _setupRole code shows it is deprecated and only calls _grantRole https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L197-L199

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

[G-11] Use newer solidity version

Solidity 0.8.11 is used in Alchemix. The latest release of solidity includes changes that can provide gas savings. Solidity version 0.8.13 can save more gas with Yul IR pipeline https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/#yul-ir-pipeline-production-ready

Recommended Mitigation Steps

Use solidity release 0.8.13 with Yul IR pipeline for gas savings

[G-12] Use Solidity errors instead of require

Solidity errors introduced in version 0.8.4 can save gas on revert conditions https://blog.soliditylang.org/2021/04/21/custom-errors/ https://twitter.com/PatrickAlphaC/status/1505197417884528640

Recommended Mitigation Steps

Replace require blocks with new solidity errors described in https://blog.soliditylang.org/2021/04/21/custom-errors/

[G-13] Use simple comparison in trinary logic

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas

The existing code is https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/TransmuterBuffer.sol#L161

credit = debt >= 0 ? 0 : SafeCast.toUint256(-debt);

A simple comparison can be used for gas savings by reversing the logic

credit = debt < 0 ? SafeCast.toUint256(-debt) : 0;

Recommended Mitigation Steps

Replace the comparison operator and reverse the logic to save gas using the suggestions above

[G-14] Use calldata instead of memory for function arguments

Using calldata instead of memory for function arguments saves gas sometimes. This can happen when a function is called externally and the memory array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). If the array is used in a for loop, arr[i] accesses the value in memory using a mload. If calldata is used instead, then 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.

At least one case of function arguments using memory instead of calldata can use this to save gas https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/CrossChainCanonicalBase.sol#L46 https://github.com/code-423n4/2022-05-alchemix/tree/main/contracts-full/CrossChainCanonicalAlchemicTokenV2.sol#L11

Source https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#use-calldata-instead-of-memory-for-function-parameters

Recommended Mitigation Steps

Change function arguments from memory to calldata

[G-15] Save gas with unchecked

Use unchecked when there is no overflow risk to save gas. Before index is decreased in remove it is checked for zero condition. This means index will not underflow and can be unchecked.

Index is decreased on line 42 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/libraries/Sets.sol#L42 index--

But line 37 handles the case when index is zero, do index will not underflow https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/libraries/Sets.sol#L42

The entire line can be unchecked because the subtraction only happens when it cannot overflow

The same logic can be used in line 44, because if self.indexes[value] is not zero, then self.values has a length greater than zero https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/libraries/Sets.sol#L44 uint256 lastIndex = self.values.length - 1;

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings unchecked { index--; } and unchecked { uint256 lastIndex = self.values.length - 1; }

[G-16] Use internal function not modifier

Using internal functions instead of modifier saves gas. Some contracts use this but not all contracts.

Source https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#dde7

Recommended Mitigation Steps

Replace the modifier in AlchemistTokenV2 with internal functions. This is done properly in AlchemistV2

Alchemix contest - 0xkatana's results

A protocol for self-repaying loans with no liquidation risk.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-05] QA Report - $178.38

Findings Information

Awards

Labels

External Links

[L-01] safeApprove is deprecated

Recommended Mitigation Steps

[L-02] Deprecated _setupRole function used

Recommended Mitigation Steps

[G-07] Gas Report - $201.23

Findings Information

Awards

Labels

External Links

[G-01] Short require strings save gas

Recommended Mitigation Steps

[G-02] Use != 0 instead of > 0

Recommended Mitigation Steps

[G-03] Redundant zero initialization

Recommended Mitigation Steps

[G-04] Use prefix not postfix in loops

Recommended Mitigation Steps

[G-05] For loop incrementing can be unsafe

Recommended Mitigation Steps

[G-06] Cache array length before loop

Recommended Mitigation Steps

[G-07] Non-public variables save gas

Recommended Mitigation Steps

[G-08] Bitshift for divide by 2

Recommended Mitigation Steps

[G-09] Add payable to functions that won't receive ETH

Recommended Mitigation Steps

[G-10] Replace _setupRole call with _grantRole

Impact

Recommended Mitigation Steps

[G-11] Use newer solidity version

Recommended Mitigation Steps

[G-12] Use Solidity errors instead of require

Recommended Mitigation Steps

[G-13] Use simple comparison in trinary logic

Recommended Mitigation Steps

[G-14] Use calldata instead of memory for function arguments

Recommended Mitigation Steps

[G-15] Save gas with unchecked

Recommended Mitigation Steps

[G-16] Use internal function not modifier

Recommended Mitigation Steps

[L-02] Deprecated `_setupRole` function used

[G-10] Replace `_setupRole` call with `_grantRole`