Alchemix contest - catchup's results

Floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Lines of code

Pretty much all the contracts, some of which are below https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1

Missing non-zero address check

It is a good practice to include non-zero address check especially when updating important addresses. I suggest to include a non-zero address check for transferOwnership()

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L39

It would be better to make transferOwnership() as a two-step process

transferOwnership() is called by the current owner to change the owner address. It can be a better approach to follow a 2-step process when updating such priviliged addresses First transaction proposes the pending owner address, second transaction which can only be called by the proposed address accepts the role. A similar 2-step approach is implemented in AlchemistV2.sol for admin update using setPendingAdmin() and acceptAdmin().

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L39

for loops can be optimized

1- For loop index increments can be made unchecked. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

There are 26 instances in 8 files, some of which are below: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L990 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1282 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1355 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1461 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1524

Recommended Mitigation Steps

I suggest to change the original code from this for (uint256 i = 0; i < depositedTokens.values.length; i++) { _preemptivelyHarvest(depositedTokens.values[i]); } to this for (uint256 i; i < depositedTokens.values.length; ) { _preemptivelyHarvest(depositedTokens.values[i]); unchecked { ++i; } }

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1458 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/EthAssetManager.sol#L566 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/ThreePoolAssetManager.sol#L771 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/ThreePoolAssetManager.sol#L901 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L534 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L549

Using != 0 is cheaper than > 0 when used on a uint in a require() statement

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/test/yearn/YearnVaultMock.sol#L53

Constant keccak variables can be changed to immutable

When variables are defined as constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed only during deployment. If the variable is going to be used within the constructor, the value assignment of the immutable variable should be executed within the constructor as well. See below for previous findings. https://github.com/code-423n4/2021-10-slingshot-findings/issues/3 https://github.com/code-423n4/2022-01-livepeer-findings/issues/172

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L22 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L25 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L21 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L24 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L27 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L22 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L25 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L28 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L31 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L34 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterV2.sol#L99 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterV2.sol#L102

Error string longer than 32 characters

Error strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

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

length variables can be cached in some for loops

bridgeTokensArray.length, _pools.length(), _yieldTokens[underlyingToken].length, registeredUnderlyings.length refer to state variables. These are used directly within the for loops, meaning they need to be read with each iteration. The values of these state variables can simply be read once and cached in the stack and the cached value can be used within the for loop.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L141 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/StakingPools.sol#L188 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/StakingPools.sol#L363 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L172 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L235 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L272 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L382 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterBuffer.sol#L479

It is better to use 1-2 for _lockState rather than 0-1

SSTORE from 1 to 2 is cheaper than SSTORE from 0 to 1. https://github.com/code-423n4/2022-01-yield-findings/issues/102

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/base/Mutex.sol#L27-L45

Missing non-zero amount checks

It is a good practice to apply non-zero amount checks for token transactions to avoid unnecessary executions.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L138 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L148 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L105 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L146 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L154 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L201

Booleans are more expensive than uint256

It is more expensive to operate using booleans because before every write operation an SLOAD is executed to read the contents of the slot. Therefore, it is cheaper to use uint256 instead of bool. On the other hand, using bool is better for the code readability. Hence, it is a tradeoff to be decided by the developers.

Lines of code

Many lines, some of which are: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L28 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L31 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L34 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L33 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L36 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L48