Store the timestamp endpoint rather than re-calculating it every time
The isTimeEnded() function does a lot of calculations every time it's called. It's called from multiple modifiers so it's important for it to be efficient. Rather than storing the duration, the code can calculate and store the ending timestamp, so isTimeEnded() can just be a direct comparison of two uint256s. The duration can be calculated by subtracting the start timestamp from the ending timestamp.
/// @notice return true if time period has ended
function isTimeEnded() public view returns (bool) {
return remainingTime() == 0;
}
/// @notice number of seconds remaining until time is up
/// @return remaining
function remainingTime() public view returns (uint256) {
return duration - timeSinceStart(); // duration always >= timeSinceStart which is on [0,d]
}
/// @notice number of seconds since contract was initialized
/// @return timestamp
/// @dev will be less than or equal to duration
function timeSinceStart() public view returns (uint256) {
if (!isTimeStarted()) {
return 0; // uninitialized
}
uint256 _duration = duration;
uint256 timePassed = block.timestamp - startTime; // block timestamp always >= startTime
return timePassed > _duration ? _duration : timePassed;
}
Lots of duplicated code between RateLimited.sol and MultiRateLimited.sol
The functionality of RateLimited.sol can be achieved by using either address(0) or address(this) as the rateLimitedAddress so having a separate RateLimited.sol contract is a waste of deployment gas.
File: contracts/utils/RateLimited.sol (Various lines throughout the file)
File: contracts/utils/MultiRateLimited.sol (Various lines throughout the file)
require()/revert() strings longer than 32 bytes cost extra gas
require(
_core.isGovernor(msg.sender) ||
_core.isGuardian(msg.sender) ||
isContractAdmin(msg.sender),
"CoreRef: Caller is not governor or guardian or admin"
);
Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
// Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.
function _setupGovernor(address governor) internal {
State variables should be cached in stack variables rather than re-reading them from storage
The instances below point to the second access of a state variable within a function.
Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.
Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
Store the timestamp endpoint rather than re-calculating it every time
The
isTimeEnded()function does a lot of calculations every time it's called. It's called from multiple modifiers so it's important for it to be efficient. Rather than storing the duration, the code can calculate and store the ending timestamp, soisTimeEnded()can just be a direct comparison of twouint256s. The duration can be calculated by subtracting the start timestamp from the ending timestamp.File: contracts/utils/Timed.sol (lines 38-59)
Lots of duplicated code between
RateLimited.solandMultiRateLimited.solThe functionality of
RateLimited.solcan be achieved by using eitheraddress(0)oraddress(this)as therateLimitedAddressso having a separateRateLimited.solcontract is a waste of deployment gas.require()/revert()strings longer than 32 bytes cost extra gasUse a more recent version of solidity
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
Use a more recent version of solidity
Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than
revert()/require()strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return valueUsing
bools for storage incurs overheadhttps://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
internalfunctions only called once can be inlined to save gasState variables should be cached in stack variables rather than re-reading them from storage
The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.
Splitting
require()statements that use&&saves gasSee this issue for an example
Usage of
uints/intssmaller than 32 bytes (256 bits) incurs overheadhttps://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
Expressions for constant values such as a call to
keccak256(), should useimmutablerather thanconstantSee this issue for a detail description of the issue
Using
privaterather thanpublicfor constants, saves gasIf needed, the value can be read from the verified contract source code
Duplicated
require()/revert()checks should be refactored to a modifier or functionState variables only set in the constructor should be declared
immutablerequire()orrevert()statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables
Use custom errors rather than
revert()/require()strings to save deployment gasNot using the named return variables when a function returns, wastes deployment gas