Alchemix contest - TerrierLover's results

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L73-L76

Vulnerability details

Impact

An attacker can make the contract unusable when totalSupply is 0. Specifically, bumpExchangeRate function does not work correctly which results in making stake, unstake and migrateSource functions that do not work as expected.

Proof of Concept

Here are steps on how the gALCX contract can be unusable.

1. gALCX contract is deployed

2. The attacker sends the ALCX token to the deployed gALCX contract directly instead of using stake function so that the following balance variable has value.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L73-L75

uint balance = alcx.balanceOf(address(this));

if (balance > 0) {

3. Since the ALCX token is given to the gALCX contract directly, totalSupply == 0 and alcx.balanceOf(address(this)) > 0 becomes true.

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

exchangeRate += (balance * exchangeRatePrecision) / totalSupply;

4. Non attackers try to call stake function, but bumpExchangeRate function fails because of (balance * exchangeRatePrecision) / totalSupply when totalSupply is 0.

5. Owner cannot call migrateSource function since bumpExchangeRate will be in the same situation mentioned in the step4 above

Tools Used

Static code analysis

Recommended Mitigation Steps

Add handling when totalSupply is 0 but alcx.balanceOf(address(this)) is more than 0.

Here are QA findings per file.

---

AlchemistV2.sol

[QA-1] No need to define constructor

AlchemistV2.sol defines constructor() as follows, but if this is upgradeable contract, this constructor is not needed.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L110

constructor() initializer {}

It simply can remove the definition of constructor.

[QA-2] Codes in _convertYieldTokensToShares and _convertSharesToYieldTokens are not consistent

_convertYieldTokensToShares and _convertSharesToYieldTokens functions have similar code structure, but _convertYieldTokensToShares accesses _yieldTokens[yieldToken].totalShares twice.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1607-L1612

function _convertYieldTokensToShares(address yieldToken, uint256 amount) internal view returns (uint256) {
    if (_yieldTokens[yieldToken].totalShares == 0) {
        return amount;
    }
    return amount * _yieldTokens[yieldToken].totalShares / _calculateUnrealizedActiveBalance(yieldToken);
}

On the other hand, _convertSharesToYieldTokens function use uint256 totalShares variable to prevent the storage access.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1620-L1626

function _convertSharesToYieldTokens(address yieldToken, uint256 shares) internal view returns (uint256) {
    uint256 totalShares = _yieldTokens[yieldToken].totalShares;
    if (totalShares == 0) {
        return shares;
    }
    return (shares * _calculateUnrealizedActiveBalance(yieldToken)) / totalShares;
}

_convertYieldTokensToShares function can follow _convertSharesToYieldTokens function.

function _convertYieldTokensToShares(address yieldToken, uint256 amount) internal view returns (uint256) {
    uint256 totalShares = _yieldTokens[yieldToken].totalShares;
    if (totalShares == 0) {
        return amount;
    }
    return amount * totalShares / _calculateUnrealizedActiveBalance(yieldToken);
}

---

[QA-3] Extra spaces exist at withdrawUnderlyingFrom and withdrawUnderlying functions where input checks happen

withdrawUnderlyingFrom and withdrawUnderlying functions have different coding styls compared with other functions in AlchemistV2.sol file.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L639-L645

function withdrawUnderlying(
    address yieldToken,
    uint256 shares,
    address recipient,
    uint256 minimumAmountOut
) external override lock returns (uint256) {
    _onlyWhitelisted();

    _checkArgument(recipient != address(0));

    _checkSupportedYieldToken(yieldToken);

    _checkLoss(yieldToken);

    uint256 amountYieldTokens = _withdraw(yieldToken, msg.sender, shares, recipient);

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L653-L668

function withdrawUnderlyingFrom(
    address owner,
    address yieldToken,
    uint256 shares,
    address recipient,
    uint256 minimumAmountOut
) external override lock returns (uint256) {
    _onlyWhitelisted();

    _checkArgument(recipient != address(0));

    _checkSupportedYieldToken(yieldToken);

    _checkLoss(yieldToken);

    _decreaseWithdrawAllowance(owner, msg.sender, yieldToken, shares);

withdrawFrom or other functions pack the codes where _only... and _check... functions are called as follows.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L609-L617

function withdrawFrom(
    address owner,
    address yieldToken,
    uint256 shares,
    address recipient
) external override lock returns (uint256) {
    _onlyWhitelisted();
    _checkArgument(recipient != address(0));
    _checkSupportedYieldToken(yieldToken);

For the better readability and consistency, withdrawUnderlying and withdrawUnderlyingFrom functions can be rewritten as follows:

function withdrawUnderlying(
    address yieldToken,
    uint256 shares,
    address recipient,
    uint256 minimumAmountOut
) external override lock returns (uint256) {
    _onlyWhitelisted();
    _checkArgument(recipient != address(0));
    _checkSupportedYieldToken(yieldToken);
    _checkLoss(yieldToken);

    uint256 amountYieldTokens = _withdraw(yieldToken, msg.sender, shares, recipient);

function withdrawUnderlyingFrom(
    address owner,
    address yieldToken,
    uint256 shares,
    address recipient,
    uint256 minimumAmountOut
) external override lock returns (uint256) {
    _onlyWhitelisted();
    _checkArgument(recipient != address(0));
    _checkSupportedYieldToken(yieldToken);
    _checkLoss(yieldToken);

    _decreaseWithdrawAllowance(owner, msg.sender, yieldToken, shares);

---

AlchemicTokenV2.sol

[QA-4] mint function in AlchemicTokenV2.sol does not behave as the comment describes

Here is mint function in AlchemicTokenV2.sol.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2.sol#L101

  /// @notice Mints tokens to `a recipient.`
  ///
  /// @notice This function reverts if `msg.sender` is not whitelisted.
  /// @notice This function reverts if `msg.sender` is paused.
  /// @notice This function reverts if `msg.sender` has exceeded their mintable ceiling.
  ///
  /// @param recipient The address to mint the tokens to.
  /// @param amount    The amount of tokens to mint.
  function mint(address recipient, uint256 amount) external onlyWhitelisted {

The comment says @notice This function reverts if `msg.sender` has exceeded their mintable ceiling. , but there is no such logic in AlchemicTokenV2.sol.

mint function in AlchemicTokenV2Base.sol has similar comment, which implements the logic related to the mintable ceiling.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2Base.sol#L116-L119

It should fix AlchemicTokenV2.sol based on the specification of this contract.

---

CrossChainCanonicalBase.sol

[QA-5] Unnecessary parenthesis is used

Following code has extra parenthesis.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L124

bridgeTokensOut -= ((bridgeTokensOut * swapFees[bridgeTokenAddress][1]) / FEE_PRECISION);

It can be rewritten like this:

bridgeTokensOut -= bridgeTokensOut * swapFees[bridgeTokenAddress][1] / FEE_PRECISION;

[QA-6] Inconsistent order of event definitions

Events are defined at the bottom of the code in CrossChainCanonicalBase.sol.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L185-L189

At other files, it follows the style guide of solidity link.

Inside each contract, library or interface, use the following order:
1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

So CrossChainCanonicalBase.sol should follow this rule by putting events definitions between state variables and modifiers.

---

CrossChainCanonicalGALCX.sol / CrossChainCanonicalAlchemicTokenV2.sol

[QA-7] Arguments naming inconsistency at initialize function

name and symbol does not have prefix _ while _bridgeTokens has one.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalGALCX.sol#L8-L9

  function initialize(
      string memory name, 
      string memory symbol, 
      address[] memory _bridgeTokens
  ) public initializer {

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalAlchemicTokenV2.sol#L9-L10

  function initialize(
      string memory name, 
      string memory symbol, 
      address[] memory _bridgeTokens
  ) public initializer {

__CrossChainCanonicalBase_init function adds prefix _ so initialize should follow __CrossChainCanonicalBase_init.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L42-L47

    function __CrossChainCanonicalBase_init(
        string memory _name,
        string memory _symbol,
        address _creatorAddress,
        address[] memory _bridgeTokens
    ) internal {

---

StakingPools.sol

[QA-8] Naming inconsistency at local variables

StakingPools.sol adds _ prefix at local variables. Here are some examples:

_totalRewardWeight, _pool and _poolId at setRewardWeights function.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L187-L191

    uint256 _totalRewardWeight = _ctx.totalRewardWeight;
    for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
      Pool.Data storage _pool = _pools.get(_poolId);

      uint256 _currentRewardWeight = _pool.rewardWeight;

_pool and _stake at deposit function.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L210-L214

    Pool.Data storage _pool = _pools.get(_poolId);
    _pool.update(_ctx);

    Stake.Data storage _stake = _stakes[msg.sender][_poolId];
    _stake.update(_pool, _ctx);

_pool and _stake at withdraw function.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L224-L228

    Pool.Data storage _pool = _pools.get(_poolId);
    _pool.update(_ctx);

    Stake.Data storage _stake = _stakes[msg.sender][_poolId];
    _stake.update(_pool, _ctx);

Other files do not add _ previx for the local variables, so StakingPools.sol should follow this pattern as well like this:

    Pool.Data storage pool = _pools.get(_poolId);
    pool.update(_ctx);

    Stake.Data storage stake = _stakes[msg.sender][_poolId];
    stake.update(_pool, _ctx);

Here is a reference of other files not using _ for local variable.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L183

Weighting storage weighting = weightings[weightToken];

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L312

uint256 localBalance = TokenUtils.safeBalanceOf(underlyingToken, address(this));

---

gALCX.sol

[QA-9] Can use Ownable.sol by OpenZeppelin instead of having custom logic

gALCX.sol defines the logic silmiar to Ownable.sol provided by OpenZeppelin.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L30-L41

    // OWNERSHIP

    modifier onlyOwner {
        require(msg.sender == owner, "Not owner");
        _;
    }

    /// @notice Transfer contract ownership
    /// @param _owner The new owner address
    function transferOwnership(address _owner) external onlyOwner {
        owner = _owner;
    }

What the above mentioned logic does is very similar to OZ's Ownable.sol, so it is worth considering using the OZ one. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

gALCX.sol does not check if _owner is address(0) or not at transferOwnership function, so it has a risk of setting address(0) which makes the admin operations of gALCX.sol unusable. If gALCX.sol keeps using the custom logic similar to OZ's Ownable.sol, it should at least check if the given address is address(0) at transferOwnership function to reduce the risk.

[QA-10] The return value of reApprove function is not used

reApprove function uses alcx.approve which returns bool success, but this success variable is not used at all.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L62-L64

function reApprove() public {
    bool success = alcx.approve(address(pools), type(uint).max);
}

It should remove setting the success boolean variable or add check if approval operation is successful or not.

    bool success = alcx.approve(address(pools), type(uint).max);
    if (!success) {
        revert SetAppropriateCustomError();
    }

[QA-11] reApprove function is not defined below // PUBLIC FUNCTIONS comment

reApprove function is public, so it should be defined below the // PUBLIC FUNCTIONS comment.

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

After the modification, the code should look like this:

    // PUBLIC FUNCTIONS

    /// @notice Approve the staking pool to move funds in this address, can be called by anyone
    function reApprove() public {
        bool success = alcx.approve(address(pools), type(uint).max);
    }

    /// @notice Claim and autocompound rewards
    function bumpExchangeRate() public {

---

[QA-12] Argument naming inconsistency at stake and unstake functions

The arguments of stake and unstake functions do not have _ at their prefix.

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

function stake(uint amount) external {

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

function unstake(uint gAmount) external {

other part of this file adds _ on their argument as follows.

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

function migrateSource(address _pools, uint _poolId) external onlyOwner {

take and unstake functions should follow this pattern.

---

[QA-13] Unnecessary parenthesis is used at bumpExchangeRate function

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

exchangeRate += (balance * exchangeRatePrecision) / totalSupply;

It can write as follows:

exchangeRate += balance * exchangeRatePrecision / totalSupply;

---

[QA-14] The code of _onlyWhitelisted function is a bit different from other files

TransmuterV2.sol has slightly different definitions of _onlyWhitelisted compared with other files.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterV2.sol#L520-L530

  function _onlyWhitelisted() internal view {
    // Check if the message sender is an EOA. In the future, this potentially may break. It is important that
    // functions which rely on the whitelist not be explicitly vulnerable in the situation where this no longer
    // holds true.
    if (tx.origin != msg.sender) {
      // Only check the whitelist for calls from contracts.
      if (!IWhitelist(whitelist).isWhitelisted(msg.sender)) {
        revert Unauthorized();
      }
    }
  }

For the reference, this is _onlyWhitelisted function defined in AlchemistV2.sol.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1701-L1712

    function _onlyWhitelisted() internal view {
        // Check if the message sender is an EOA. In the future, this potentially may break. It is important that functions
        // which rely on the whitelist not be explicitly vulnerable in the situation where this no longer holds true.
        if (tx.origin == msg.sender) {
          return;
        }

        // Only check the whitelist for calls from contracts.
        if (!IWhitelist(whitelist).isWhitelisted(msg.sender)) {
          revert Unauthorized();
        }
    }

The behavior is same, but for the consistency, it is worh adjusting _onlyWhitelisted function at TransmuterV2.sol to follow other codebase.

Here are gas optimizations per file.

---

AlchemistV2.sol

[Gas-1] constructor is not needed

constructor is defined, but this is upgradeable contract and the constructor will not be used.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L110

constructor() initializer {}

Removing the definition of constructor may reduce the deployment gas cost.

---

[Gas-2] Use != 0 instead of > 0 on uint256 variables

Following codes use != 0 for the zero value check like this.

_checkArgument(config.creditUnlockBlocks > 0);

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L353 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L466 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L678 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L692 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L707 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L810 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L846 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1103 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1267 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1531

uint256 variables will not be less than 0, so it simply needs to check != 0 like this which results in decreasing the gas cost.

_checkArgument(config.creditUnlockBlocks != 0);

---

[Gas-3] No neet to set 0 on uint256 variables

Following codes set 0 on uint256 variables. The default value of uint256 variable is 0, so there is no need to set 0. Not setting 0 can reduce the deployment gas fee.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L990 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1282 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1355 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1458 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1461 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemistV2.sol#L1524

For loop can be rewritten like this:

for (uint256 i; i < depositedTokens.values.length; i++) {
    _preemptivelyHarvest(depositedTokens.values[i]);
}

For the local uint256 variable, it can be defined like this:

function _totalValue(address owner) internal view returns (uint256) {
    uint256 totalValue;

---

AlchemicTokenV2.sol

[Gas-4] Use uint256 instead of uint

uint is used at the argument of setMaxFlashLoan function.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2.sol#L164

function setMaxFlashLoan(uint _maxFlashLoanAmount) external onlyAdmin {

Other parts of code use uint256 consistently, so setMaxFlashLoan should use uint256 at its argument as well.

function setMaxFlashLoan(uint256 _maxFlashLoanAmount) external onlyAdmin {

[Gas-5] newAllowance variable is not needed

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2.sol#L155-L157

uint256 newAllowance = allowance(account, msg.sender) - amount;

_approve(account, msg.sender, newAllowance);

newAllowance is defined, but it is only used once. Removing the definition of newAllowance can reduce the gas cost as follows:

_approve(account, msg.sender, allowance(account, msg.sender) - amount);

---

CrossChainCanonicalBase.sol

[Gas-6] No need to set 0 on uint256 variables

Following codes set 0 on uint256 variables. The default value of uint256 variable is 0, so there is no need to set 0. Not setting 0 can reduce the deployment gas fee.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L57 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L141

It can be rewritten like this:

for (uint i; i < bridgeTokensArray.length; i++){

---

[Gas-7] Use uint256 instead of uint

Following codes use uint, but it can use uint256.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L141

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L189

---

[Gas-8] mapping(address => bool) public bridgeTokens seems not be used at all

mapping(address => bool) public bridgeTokens is defined but it is not used by anywhere except for the __CrossChainCanonicalBase_init.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L25

bridgeTokensArray state variable is used instead. It should either remove bridgeTokens state variable and relevant codebase or use bridgeTokens state variable more efficiently.

If bridgeTokens state variable is updated appropriately, bridgeTokens may be able to be used when checking if the target token is present or not instead of looping through the bridgeTokensArray.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L140-L145

// Make sure the token is not already present
for (uint i = 0; i < bridgeTokensArray.length; i++){ 
    if (bridgeTokensArray[i] == bridgeTokenAddress) {
        revert IllegalState();
    }
}

---

StakingPools.sol

[Gas-9] Using error statement

Only StakingPools.sol uses require and does not use custome errors as follows while other files use it.

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

Using the custom errors can reduce the gas cost, so it is worth using at StakingPools.sol as well.

---

[Gas-10] _pendingGovernance variable seems not needed at acceptGovernance function

_pendingGovernance variable is defined at acceptGovernance function but this seems not needed.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L130-L137

  function acceptGovernance() external {
    require(msg.sender == pendingGovernance, "StakingPools: only pending governance");

    address _pendingGovernance = pendingGovernance;
    governance = _pendingGovernance;

    emit GovernanceUpdated(_pendingGovernance);
  }

It can be rewritten like this which can decrease the gas cost:

  function acceptGovernance() external {
    require(msg.sender == pendingGovernance, "StakingPools: only pending governance");

    governance = pendingGovernance;

    emit GovernanceUpdated(pendingGovernance);
  }

---

[Gas-11] No neet to set 0 on uint256 variables

Following codes set 0 on uint256 variables. The default value of uint256 variable is 0, so there is no need to set 0. Not setting 0 can reduce the deployment gas fee.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L188 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/StakingPools.sol#L363

It can rewrite like this:

for (uint256 _poolId; _poolId < _pools.length(); _poolId++) {

---

[Gas-12] Use uint256 instead of uint

Throughout the codebase, it uses uint256. However, following parts use uint.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L50 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L56 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L73 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L93 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L85 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L100

It should consider using uint256 instead of uint.

---

[Gas-13] Use != 0 instead of > 0

balance is uint256 so it can use != 0 instead of > 0.

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

if (balance > 0) {

---

TransmuterV2.sol

[Gas-14] constructor is not needed

constructor is defined, but this is upgradeable contract and the constructor will not be used.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterV2.sol#L141

constructor() initializer {}

Removing the definition of constructor may reduce the deployment gas cost.

---

[Gas-15] Setting false at isPaused is not needed

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterV2.sol#L162

isPaused = false;

The default value of isPaused state variable is false. So there is no need to set false at initialize function.

---

TransmuterBuffer.sol

[Gas-16] weight variable is not needed

weight is defined at getWeight function, but it is not needed.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L134

    /// @inheritdoc ITransmuterBuffer
    function getWeight(address weightToken, address token)
        external
        view
        override
        returns (uint256 weight)
    {
        return weightings[weightToken].weights[token];
    }

It can write like this:

    /// @inheritdoc ITransmuterBuffer
    function getWeight(address weightToken, address token)
        external
        view
        override
        returns (uint256)
    {
        return weightings[weightToken].weights[token];
    }

[Gas-17] minimumAmountOut is not needed when wantShares is 0

minimumAmountOut is only needed when wantShares is more than 0.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L520-L522

        uint256 minimumAmountOut = amountUnderlying - amountUnderlying * 100 / 10000;
        if (wantShares > 0) {
            IAlchemistV2(alchemist).withdrawUnderlying(token, wantShares, address(this), minimumAmountOut);
        }

It can put the logic to get minimumAmountOut inside the if statement.

        if (wantShares > 0) {
            IAlchemistV2(alchemist).withdrawUnderlying(token, wantShares, address(this), amountUnderlying - amountUnderlying * 100 / 10000);
        }

---

[Gas-18] No need to set 0 on uint256 variables

Following codes set 0 on uint256 variables. The default value of uint256 variable is 0, so there is no need to set 0. Not setting 0 can reduce the deployment gas fee.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L172 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L186 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L235 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L242 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L272 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L382 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L387 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L534 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L549

---

[Gas-19] Use != 0 instead of > 0 on uint256 variables

Following codes use > 0 on uint256 variables. uint256 variables will not be less than 0, so it simply needs to check != 0 to reduce the gas cost.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L521 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L544 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/TransmuterBuffer.sol#L556