Alchemix contest - 0x1f8b's results

Lack of empty address checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Source code:

• FuseTokenAdapterV1.sol#L47-L49
• WstETHAdapterV1.sol#L41-L48
• RETHAdapterV1.sol#L39-L41
• VesperAdapterV1.sol#L37-L39
• YearnTokenAdapter.sol#L21
• WETHGateway.sol#L23-L24
• TransmuterConduit.sol#L24
• TransmuterBuffer.sol#L88
• TransmuterV2.sol#L144-L147
• StakingPools.sol#L108
• EthAssetManager.sol#L201-L212
• ThreePoolAssetManager.sol#L236-L248
• AlchemistV2.sol#L258-L264

As stated in the OpenZeppelin source comments, the OpenZeppeling ERC20 safeApprove() function has been deprecated.

Using this deprecated function could result in accidental reverts and possibly fund locking. The deprecation of this function is discussed in greater depth in the OZ issue #2219.

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

Source code:

• FuseTokenAdapterV1.sol#L71
• WstETHAdapterV1.sol#L105
• WstETHAdapterV1.sol#L129
• VesperAdapterV1.sol#L62
• YearnTokenAdapter.sol#L32
• EthAssetManager.sol#L576-L577
• EthAssetManager.sol#L620-L621
• EthAssetManager.sol#L671-L672
• ThreePoolAssetManager.sol#L782-L783
• ThreePoolAssetManager.sol#L838-L839
• ThreePoolAssetManager.sol#L879-L880
• ThreePoolAssetManager.sol#L908-L909
• ThreePoolAssetManager.sol#L987-L988

Unsafe ERC20 calls

The following code doesn't check the result of the transferFrom or approve calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Source code:

• AutoleverageCurveFactoryethpool.sol#L26
• AutoleverageCurveMetapool.sol#L16
• gALCX.sol#L63
• WETHGateway.sol#L36
• AutoleverageBase.sol#L62

Unsecure Ownership change

The modification of an owner is a delicate procedure, as it may jeopardize the governance of our contract and therefore the project. As a result, it is advised that the owner's modification logic be changed to one that allows for verification that the new owner is valid and exists.

It is required to establish a logic for the owner's modification in which a new owner is proposed first, the owner approves the suggestion, and we ensure that the new owner's address is written correctly.

Source code:

• gALCX.sol#L40

Possible Rogue pool

The owner of the WETHGateway contract has the ability to extract all existing WETH from the contract via the refreshAllowance method. These capabilities must be limited in favor of decentralization, in order to avoid loss of reputation or user confidence.

Source code:

• WETHGateway.sol#L36

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Source code:

• Multicall.sol#L14
• CrossChainCanonicalBase.sol#L57
• CrossChainCanonicalBase.sol#L141
• TransmuterBuffer.sol#L172
• TransmuterBuffer.sol#L186
• TransmuterBuffer.sol#L235
• TransmuterBuffer.sol#L242
• TransmuterBuffer.sol#L272
• TransmuterBuffer.sol#L382
• TransmuterBuffer.sol#L387
• TransmuterBuffer.sol#L479
• StakingPools.sol#L188
• StakingPools.sol#L363
• EthAssetManager.sol#L214
• EthAssetManager.sol#L567
• ThreePoolAssetManager.sol#L250
• ThreePoolAssetManager.sol#L254
• ThreePoolAssetManager.sol#L353
• ThreePoolAssetManager.sol#L773
• ThreePoolAssetManager.sol#L902
• AlchemistV2.sol#L990
• AlchemistV2.sol#L1282
• AlchemistV2.sol#L1355
• AlchemistV2.sol#L1461
• AlchemistV2.sol#L1524

Reduce math operations

Use scientific notation (e.g. 10E18) rather than exponentiation (e.g. 10**18)

Source references:

• LiquidityMath.sol#L41
• LiquidityMath.sol#L46

Use constants to avoid extra calculation costs

There are logics that return constant values and have not been stored in constants, being able to save gas when computing them.

Source references:

• keccak256(abi.encodePacked("contract.address", "rocketTokenRETH")) at RocketPool.sol#L14

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

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).

Source references:

• AlchemicTokenV1.sol#L81
• StakingPools.sol#L106
• StakingPools.sol#L124
• StakingPools.sol#L131
• StakingPools.sol#L160
• StakingPools.sol#L183