search

code-423n4 / 2022-02-pooltogether-findings

0 stars 0 forks source link

Gas Optimizations #15

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    TWABDelegator.sol, _computeLockUntil
    TWABDelegator.sol, _requireDelegateeNotZeroAddress
    TWABDelegator.sol, _executeCall
    TWABDelegator.sol, _transfer
    TWABDelegator.sol, _requireContract
    TWABDelegator.sol, _computeAddress
    TWABDelegator.sol, _requireAmountGtZero
    TWABDelegator.sol, _requireDelegatorNotZeroAddress
    LowLevelDelegator.sol, _computeAddress
    TWABDelegator.sol, _transferCall
    TWABDelegator.sol, _requireRecipientNotZeroAddress
    PermitAndMulticall.sol, _permitAndMulticall
    TWABDelegator.sol, _requireDelegationUnlocked
    TWABDelegator.sol, _requireLockDuration
    TWABDelegator.sol, _requireDelegatorOrRepresentative
    LowLevelDelegator.sol, _createDelegation
    PermitAndMulticall.sol, _multicall
    TWABDelegator.sol, _delegateCall
    LowLevelDelegator.sol, _computeSalt
    Delegation.sol, _executeCall

Title: Unnecessary equals boolean Severity: GAS

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    TWABDelegator.sol, 583: _delegator == msg.sender || representatives[_delegator][msg.sender] == true,

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas. 

    delegationInstance in LowLevelDelegator.sol

Title: Unnecessary array boundaries check when loading an array element twice Severity: GAS

There are places in the code (especially in for-each loops) that loads the same array element more than once. 
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check: 

    Delegation.sol.executeCalls - double load of calls[i]

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.

    LowLevelDelegator.sol, _createDelegation
    LowLevelDelegator.sol, _computeSalt
    PermitAndMulticall.sol, _permitAndMulticall
    LowLevelDelegator.sol, _computeAddress

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from: 

for (uint256 i=0; i<array.length; i++) { ... }

to: 

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

    Delegation.sol, calls, 42
    PermitAndMulticall.sol, _data, 33

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    PermitAndMulticall.sol, line 4, import "@openzeppelin/contracts/token/ERC20/extensions/draft-IERC20Permit.sol";

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword: 

    change to prefix increment and unchecked: Delegation.sol, i, 42
    change to prefix increment and unchecked: PermitAndMulticall.sol, i, 33

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

    TWABDelegator.sol, _computeLockUntil, { return uint96(block.timestamp) + _lockDuration; }
    LowLevelDelegator.sol, _computeSalt, { return keccak256(abi.encodePacked(_delegator, _slot)); }
    TWABDelegator.sol, _computeAddress, { return _computeAddress(_computeSalt(_delegator, bytes32(_slot))); }
    LowLevelDelegator.sol, _computeAddress, { return address(delegationInstance).predictDeterministicAddress(_salt, address(this)); }

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions. 

    Delegation.sol, initialize

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    TWABDelegator.constructor (name_)
    TWABDelegator.constructor (symbol_)
    Delegation._executeCall (data)
    TWABDelegator._executeCall (_data)

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    TWABDelegator.sol, _transferCall
    PermitAndMulticall.sol, _multicall
    TWABDelegator.sol, _requireDelegatorNotZeroAddress
    Delegation.sol, _executeCall

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    PermitAndMulticall.sol, 33
    Delegation.sol, 42

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    TWABDelegator.sol (L#511) - return uint96(block.timestamp) + _lockDuration;

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list: 

    Solidity file: TWABDelegator.sol, In line 582, Require message length to shorten: 34, The message: TWABDelegator/not-delegator-or-rep

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    TWABDelegator.sol, 273: change '_lockDuration > 0' to '_lockDuration != 0'
    TWABDelegator.sol, 601: change '_amount > 0' to '_amount != 0'
PierrickGT commented 2 years ago

Another automatic report by the same warden. I did fix some issues though.

About Internal functions to private: I did some gas golfing in Remix and private functions are actually more gas consuming than internal ones. It is also easier to maintain contract with internal functions, since we can inherit from it in another contract without having to change the visibility of the functions from private to internal.

About Unnecessary equals boolean: Fixed in this commit: https://github.com/pooltogether/v4-twab-delegator/pull/20/commits/3311d6cd63c52b107b5855a8c7a26daeaa8944cb

About State variables that could be set immutable: Irrelevant report since immutable variables cannot be read during contract creation time.

About Unnecessary array boundaries check when loading an array element twice: Already fixed in https://github.com/pooltogether/v4-twab-delegator/pull/18

About Unnecessary functions: These functions are definitely used.

About Caching array length can save gas: Fixed in this PR: https://github.com/pooltogether/v4-twab-delegator/pull/20

About Unused imports: Import used on this line: https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/PermitAndMulticall.sol#L47

About Prefix increments are cheaper than postfix increments: A bit less readable, so we won't fix it.

About Consider inline the following functions to save gas: We use Prettier to lint our code and prefer to have a readable codebase instead of long one line functions.

About Public functions to external: Fixed in this commit: https://github.com/pooltogether/v4-twab-delegator/pull/20/commits/321af4041805145a06cd000c2d1b0651237277de

About Use calldata instead of memory: All these functions require memory to be passed and not calldata.

About Inline one time use functions: It is easier for code maintenance to use reusable helper functions instead of inlining them.

About Unnecessary index init: Fixed in this PR: https://github.com/pooltogether/v4-twab-delegator/pull/18

About Use unchecked to save gas for certain additive calculations that cannot overflow Fixed in this commit: https://github.com/pooltogether/v4-twab-delegator/pull/20/commits/82049eff4c860c0772937f72edf64b5c8a8e8960

About Short the following require messages: Fixed in this commit: https://github.com/pooltogether/v4-twab-delegator/pull/20/commits/a1c54fa2844b446261d1f91b33c2311aeff5a219

About Use != 0 instead of > 0: We are not going to fix this one, since our code would be less legible and also because there are no concrete examples proving that it is indeed less gas consuming than a comparison operator.