Gas Optimizations

[code423n4]

Gas optimization report

[G-01] No need to initialize uint to 0

There is no need to initialize a uint to 0, since this will be done by default when declaring the variable. In fact it will cost a small amount of gas to initialize the value “twice”. An example can be found in the contract at AuraLocker.sol L72.

The initialization should instead look like this:

uint256 public queuedCvxCrvRewards;

Variables initialized to 0 can also be found several times in AuraBalRewardPool.sol and AuraMerkleDrop.sol.

[G-02] Optimize for loops

The loops found in e.g. AuraLocker.sol can be optimized to save gas. They could look like this found at L173-L185:

uint256 rewardTokensLength = rewardTokens.length;
for (uint256 i = 0; i < rewardTokensLength; i++) {
    address token = rewardTokens[i];
    uint256 newRewardPerToken = _rewardPerToken(token);
    rewardData[token].rewardPerTokenStored = newRewardPerToken.to96();
    rewardData[token].lastUpdateTime = _lastTimeRewardApplicable(rewardData[token].periodFinish).to32();
    if (_account != address(0)) {
        userData[_account][token] = UserData({
            rewardPerTokenPaid: newRewardPerToken.to128(),
            rewards: _earned(_account, token, userBalance.locked).to128()
        });
    }
}

3 modifications can be done to this loop to safe gas:

1. There is no need to initialize the counter i to 0, since this is automatically done when declaring a uint (the same argument as G-01).
2. Preincrement ++i is cheaper than postincrement i++ , since the latter will make use of a temporary variable to store the returned value in before incrementing it. This can be saved by doing a preincrement, which increments the value and then returns it.
3. The likelihood of a index inside a array is next to nothing, there is no need to use checked arithmetic, and this we can do unchecked arithmetic when incrementing the counter/array index i.

With the above suggestions the for-loop above can be rewritten into:

uint256 rewardTokensLength = rewardTokens.length;
for (uint256 i; i < rewardTokensLength) {
    address token = rewardTokens[i];
    uint256 newRewardPerToken = _rewardPerToken(token);
    rewardData[token].rewardPerTokenStored = newRewardPerToken.to96();
    rewardData[token].lastUpdateTime = _lastTimeRewardApplicable(rewardData[token].periodFinish).to32();
    if (_account != address(0)) {
        userData[_account][token] = UserData({
            rewardPerTokenPaid: newRewardPerToken.to128(),
            rewards: _earned(_account, token, userBalance.locked).to128()
        });
    }
		unchecked { ++i; }
}

[G-03] Redundant division followed by multiplication

Throughout the contract AuraLocker.sol there are multiple places where the block timestamp is divided and multiplied by the same constant. Division immediately followed by a multiplication (or vice versa) with the same number equals each other out, and hence both operations are redundant. An example can be seen at L162:

uint256 currentEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration);

Where the div() and mul() will be a waste of gas since they use the same constant, and thus can be rewritten to:

uint256 currentEpoch = block.timestamp

[G-04] Variable minterMinted initialized with unused value

The state variable minterMinted in the contract Aura.sol is initialized with the value of type(uint256).max however, the variable is set to 0 in the contract’s init() function. The variable is used in two other places, which required the init() method to have been called first. Thus the initial value of type(uint256).max is never used, and is hence a waste of gas. The variable is declared at L33 and should instead be declared as:

uint256 private minterMinted;

This also means that the assignment of 0 to the variable in the init() method can be removed to save further gas. Hence the line L74 can be deleted, so the init() method looks like this:

function init(
        address _to,
        uint256 _amount,
        address _minter
    ) external {
        require(msg.sender == operator, "Only operator");
        require(totalSupply() == 0, "Only once");
        require(_amount > 0, "Must mint something");
        require(_minter != address(0), "Invalid minter");

        _mint(_to, _amount);
        updateOperator();
        minter = _minter;

        emit Initialised();
    }

[G-05] Cache merkleRoot in AuraMerkleDrop

In the method claim() in the contract AuraMerkleDrop.sol the variable merkleRoot can be cached to save some gas. The state variable is read 2 times in the method, and by caching it in memory it is possible to switch one of the expensive SLOAD operations to a MLOAD/MSTORE operation which is way cheaper.

The code L119-L126:

require(merkleRoot != bytes32(0), "!root"); // SLOAD1
require(block.timestamp > startTime, "!started");
require(block.timestamp < expiryTime, "!active");
require(_amount > 0, "!amount");
require(hasClaimed[msg.sender] == false, "already claimed");

bytes32 leaf = keccak256(abi.encodePacked(msg.sender, _amount));
require(MerkleProof.verify(_proof, merkleRoot, leaf), "invalid proof"); //SLOAD2

Can be changed into:

bytes32 cachedMerkleRoot = merkleRoot; // SLOAD + MSTORE
require(cachedMerkleRoot != bytes32(0), "!root"); // MLOAD1
require(block.timestamp > startTime, "!started");
require(block.timestamp < expiryTime, "!active");
require(_amount > 0, "!amount");
require(hasClaimed[msg.sender] == false, "already claimed");

bytes32 leaf = keccak256(abi.encodePacked(msg.sender, _amount));
require(MerkleProof.verify(_proof, cachedMerkleRoot, leaf), "invalid proof"); // MLOAD2

[G-06] Cache auraLocker AuraMerkleDrop

In the method claim() in the contract AuraMerkleDrop.sol the state variable auraLocker is read 2 times inside the if clause, and by caching it in memory it is possible to switch one of the expensive SLOAD operations to a MLOAD/MSTORE operation which is way cheaper.

There is no reason to cache auraLocker outside of the if-statement, since the else clause only reads the variable once. If we cache it outside the if-statement we would make the else part a bit more expensive.

The code L130-L139:

if (_lock) {
    aura.safeApprove(address(auraLocker), 0); // SLOAD1
    aura.safeApprove(address(auraLocker), _amount); // SLOAD2
    auraLocker.lock(msg.sender, _amount);
} else {
    // If there is an address for auraLocker, and not locking, apply 20% penalty
    uint256 penalty = address(auraLocker) == address(0) ? 0 : (_amount * 2) / 10;
    pendingPenalty += penalty;
    aura.safeTransfer(msg.sender, _amount - penalty);
}

Can be changed into:

if (_lock) {
	IAuraLocker cachedAuraLocker = auraLocker; // SLAOD1
  aura.safeApprove(address(cachedAuraLocker), 0); // MLOAD1
  aura.safeApprove(address(cachedAuraLocker), _amount); // MLOAD2
  auraLocker.lock(msg.sender, _amount);
} else {
    // If there is an address for auraLocker, and not locking, apply 20% penalty
    uint256 penalty = address(auraLocker) == address(0) ? 0 : (_amount * 2) / 10;
    pendingPenalty += penalty;
    aura.safeTransfer(msg.sender, _amount - penalty);
}

[0xMaharishi]

G-3 and G-4 are invalid, otherwise these are fair reports