Aura Finance contest - FSchmoede's results

QA report

[N-01] Use capital letters for constants and immutables

The convention for constants in Solidity is to use capital letters, see Solidity style guide.

Examples of constants not using this convention can be found in AuraLocker.sol L73, L81, L83, L104, L105, L107, L109 and L119.

This means that variables like these:

uint256 public constant denominator = 10000;
address public immutable cvxcrvStaking;

Should be named to:

uint256 public constant DENOMINATOR= 10000;
address public immutable CVXCRV_STAKING;

[N-02] Use time units instead of calculations

The usage of Solidity’s time units can increase the readability of the code. For example in the AuraLocker.sol contract at L81-L83:

uint256 public constant rewardsDuration = 86400 * 7;
//     Duration of lock/earned penalty period
uint256 public constant lockDuration = rewardsDuration * 17;

Can be rewritten into:

uint256 public constant rewardsDuration = 1 weeks;
//     Duration of lock/earned penalty period
uint256 public constant lockDuration = rewardsDuration * 17;

Which makes it much more clear that rewardsDuration is 1 week and lockDuration is 17 weeks.

[N-03] Declaration of functions out of order

In some contracts the order of the methods seems to be more or less random, which makes the contract hard to read through. An example of such contract could be AuraBalRewardPool.sol. The Solidity docs have a style guide which gives some good guidelines on the order of which methods should be declared to improve readability, see the style guide.

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