Governable Code Style Findings

GEL-01C: Inefficient Library Implementation

Type	Severity	Location
Gas Optimization		Governable.sol:L18, L25, L40, L50, L53

Description:

The LibGovernance implementation is highly inefficient as it will invoke the Governable::governanceStorage function multiple times, including within loops, inefficiently.

Example:

Copy
12/**
13 * @notice Verifies the message hash against the signatures. Requires a majority.
14 * @param _ethHash hash to verify
15 * @param _signatures governance hash signatures
16 */
17function onlyConsensus(bytes32 _ethHash, bytes[] memory _signatures) internal view {
18    uint256 members = LibGovernance.membersCount();
19    require(_signatures.length <= members, 'Governance: Invalid number of signatures');
20    require(_signatures.length > members / 2, 'Governance: Invalid number of signatures');
21
22    address[] memory signers = new address[](_signatures.length);
23    for (uint256 i = 0; i < _signatures.length; i++) {
24        address signer = ECDSA.recover(_ethHash, _signatures[i]);
25        require(LibGovernance.isMember(signer), 'Governance: invalid signer');
26        for (uint256 j = 0; j < i; j++) {
27            require(signer != signers[j], 'Governance: duplicate signatures');
28        }
29        signers[i] = signer;
30    }
31}
32
33/**
34 * @notice Verifies the message hash against the signatures. Requires a majority. Burns a nonce.
35 * @param _ethHash hash to verify
36 * @param _signatures governance hash signatures
37 */
38modifier onlyConsensusNonce(bytes32 _ethHash, bytes[] calldata _signatures) {
39    onlyConsensus(_ethHash, _signatures);
40    LibGovernance.governanceStorage().administrativeNonce.increment();
41    _;
42}
43
44/**
45 * @notice Verifies the message hash against the signatures. Requires a majority. Burns the hash.
46 * @param _ethHash hash to verify
47 * @param _signatures governance hash signatures
48 */
49modifier onlyConsensusHash(bytes32 _ethHash, bytes[] memory _signatures) {
50    LibGovernance.Storage storage gs = LibGovernance.governanceStorage();
51    require(!gs.hashesUsed[_ethHash], 'Governance: message hash already used');
52    gs.hashesUsed[_ethHash] = true;
53    onlyConsensus(_ethHash, _signatures);
54    _;
55}

Recommendation:

We advise the LibGovernance library to be revised, accepting a LibGovernance::Storage argument and thus ensuring that the same Storage pointer can be re-used across function invocations.

This can be coupled with a using LibGovernance for LibGovernance.Storage statement thereby permitting the functions to be accessed from the gs local instance directly.

Alleviation (54fd570de2):

The exhibit has been partially alleviated by optimizing a portion of the instances highlighted.

Alleviation (d7618eed4e):

The Governable::onlyConsensus function was updated to accept the LibGovernance::Storage pointer as an argument, further optimizing its gas cost per our original exhibit and thus addressing this exhibit in full.

GEL-02C: Loop Iterator Optimizations

Type	Severity	Location
Gas Optimization		Governable.sol:L23, L26

Description:

The linked for loops increment / decrement their iterator "safely" due to Solidity's built - in safe arithmetics (post-0.8.X).

Example:

Copy
23for (uint256 i = 0; i < _signatures.length; i++) {

Recommendation:

We advise the increment / decrement operations to be performed in an unchecked code block as the last statement within each for loop to optimize their execution cost.

Alleviation (54fd570de24631ca65a7cea022aebe43225a08c7):

The optimization has been applied as advised to the first referenced instance whilst the second one has been omitted from the codebase rendering this exhibit fully addressed.

GEL-03C: Non-Standard Invocation of Library

Type	Severity	Location
Code Style		Governable.sol:L25

Description:

The referenced statement represents a library function invocation without an inferred argument.

Example:

Copy
25require(LibGovernance.isMember(signer), 'Governance: invalid signer');

Recommendation:

We advise a using LibGovernance for address statement to be introduced to the contract, permitting the LibGovernance::isMember function to be invoked on the signer argument directly (i.e. signer.isMember()).

Alleviation (54fd570de24631ca65a7cea022aebe43225a08c7):

The non-standard invocation style has been addressed as advised.