Gas Optimizations #335

code423n4 · 2022-08-06T18:50:06Z

Gas

Gas

1. Is `DebtToken` needed?

It is possible that the DebtToken is not necessary, its functionality is to keep track of the amount of debt that an address has, but this debt is not linked to the projects, so either a DebtToken contract is created per project, or this amount is a sum of all the debts of the account. It is possible that the project can be redesigned to only have a debt mapping and thus eliminate this contract, with the consequent gas savings and also improving debt traceability.

Affected source code:

DebtToken.sol#L11

2. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In this case, the statements are short, so it is recommended to use custom errors.

Affected source code:

3. Remove None

According to the documentation The status None is not defined:

Task	Entity defined by a cost, a subcontractor, and a status. a task can be **Inactive, Active or Complete**. Tasks can be flagged as Allocated when budget for this task has bee provisioned. When subcontractor confirms the assignment on a task it is being flagged as SCConfirmed

if we use Inactive in the first position, or we remove None like this:

enum TaskStatus {
-   None,
    Inactive,
    Active,
    Complete
}

function initialize(Task storage _self, uint256 _cost) public {
    _self.cost = _cost;
-   _self.state = TaskStatus.Inactive;
    _self.alerts[uint256(Lifecycle.None)] = true;
}

We can avoid to set the Inactive state in every initialize calls.

Affected source code:

Tasks.sol#L24-L25

4. `++i` costs less gas compared to `i++` or `i += 1`

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

5. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

6. Don't use storage variables for loops condition

It's cheaper to cache the storage variable inside a local variable and iterate over it.

Affected source code:

Community.sol#L624

7. Improve deserialization

It's possible to save a lot of logic (and gas) if decode the _actionType as an enum.

        // Decode params from _data
        (
            address _project,
            uint256 _taskID,
-           uint8 _actionType,
+           ActionType _actionType,
            bytes memory _actionData,
            bytes memory _reason
-       ) = abi.decode(_data, (address, uint256, uint8, bytes, bytes));
+       ) = abi.decode(_data, (address, uint256, ActionType, bytes, bytes));

-       // Revert if _actionType is invalid
-       require(
-           _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
-           "Disputes::!ActionType"
-       );

        // Store dispute details
        Dispute storage _dispute = disputes[disputeCount];
        _dispute.status = Status.Active;
        _dispute.project = _project;
        _dispute.taskID = _taskID;
        _dispute.raisedBy = _signer;
-       _dispute.actionType = ActionType(_actionType);
+       _dispute.actionType = _actionType;
        _dispute.actionData = _actionData;

        // Increment dispute counter and emit event
        emit DisputeRaised(disputeCount++, _reason);

Affected source code:

Disputes.sol#L96-L121

8. Reorder conditionals to be cheaper

It's better to check first the internal checks, and after it, do the external calls.

-       bool _result = _projectInstance.builder() == _address ||
+       bool _result = _sc == _address ||
            _projectInstance.contractor() == _address ||
+           _projectInstance.builder() == _address;
-           _sc == _address;
        require(_result, "Disputes::!Member");

Affected source code:

Disputes.sol#L180-L183

9. Save storage access

The method mintNFT returns the projectCount storage value as memory variable, so it's better to reuse this value instead of call the storage again.

-       mintNFT(_sender, string(_hash));
        // Update project related mappings
        projects[projectCount] = _project;
-       projectTokenId[_project] = projectCount;
+       projectTokenId[_project] = mintNFT(_sender, string(_hash));

Affected source code:

HomeFi.sol#L225-L229

10. Reduce code size

Changing the order of the signatures it's possible to reduce the logic and the contract size.

+           checkSignatureValidity(contractor, _hash, _signature, 0);
-           if (contractorDelegated) {
+           if (!contractorDelegated) {
+               checkSignatureValidity(builder, _hash, _signature, 1);
-               //  Check contractor's signature
-               checkSignatureValidity(contractor, _hash, _signature, 0);
            }
-           // When builder has not delegated rights to contractor
-           else {
-               // Check for both B and GC signatures
-               checkSignatureValidity(builder, _hash, _signature, 0);
-               checkSignatureValidity(contractor, _hash, _signature, 1);
-           }

Affected source code:

Project.sol#L805-L815

+           checkSignatureValidity(contractor, _hash, _signature, 0);
+           checkSignatureValidity(_sc, _hash, _signature, 1);
-           if (contractorDelegated) {
+           if (!contractorDelegated) {
+               checkSignatureValidity(builder, _hash, _signature, 2);
-               // Check for GC and SC sign
-               checkSignatureValidity(contractor, _hash, _signature, 0);
-               checkSignatureValidity(_sc, _hash, _signature, 1);
            }
-           // When builder has not delegated rights to contractor
-           else {
-               // Check for B, SC and GC signatures
-               checkSignatureValidity(builder, _hash, _signature, 0);
-               checkSignatureValidity(contractor, _hash, _signature, 1);
-               checkSignatureValidity(_sc, _hash, _signature, 2);
-           }

Affected source code:

Project.sol#L850-L861

11. Improve logic

It doesn't need to delete or check the signature always. It's better to do the logic according that what happened.

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
+       if (approvedHashes[_address][_hash]) {
+           delete approvedHashes[_address][_hash];
+           return;
+       }
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
+       if (_recoveredSignature == address(0) || _recoveredSignature != _address) revert("Project::invalid signature");
-       require(
-           _recoveredSignature == _address || approvedHashes[_address][_hash],
-           "Project::invalid signature"
-       );
-       // delete from approvedHash
-       delete approvedHashes[_address][_hash];
    }

Affected source code:

Project.sol#L875-L892

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal virtual {
+       if (approvedHashes[_address][_hash]) {
+           delete approvedHashes[_address][_hash];
+           return;
+       }
        // Decode signer
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );

        // Revert if decoded signer does not match expected address
-       // Or if hash is not approved by the expected address.
+       if (_recoveredSignature == address(0) || _recoveredSignature != _address) revert("Community::invalid signature");
-       require(
-           _recoveredSignature == _address || approvedHashes[_address][_hash],
-           "Community::invalid signature"
-       );
- 
-       // Delete from approvedHash. So that signature cannot be reused.
-       delete approvedHashes[_address][_hash];
    }

Affected source code:

Community.sol#L871-L893

It's not needed to emit the '0x' value, and it's cheaper to emit an empty one.

Affected source code:

-       _reduceDebt(_communityID, _project, _repayAmount, "0x");
+       _reduceDebt(_communityID, _project, _repayAmount, "");

Community.sol#L468

12. Use `memory` instead `storage`

Since all the values of the ProjectDetails struct are returned, it is cheaper to avoid continuous storage accesses and use memory.

    function projectDetails(uint256 _communityID, address _project)
        external
        view
        virtual
        override
        returns (
            uint256,
            uint256,
            uint256,
            uint256,
            bool,
            uint256,
            uint256,
            uint256
        )
    {
+       ProjectDetails memory _communityProject = _communities[_communityID]
-       ProjectDetails storage _communityProject = _communities[_communityID]
            .projectDetails[_project];

        return (
            _communityProject.apr,
            _communityProject.lendingNeeded,
            _communityProject.totalLent,
            _communityProject.publishFee,
            _communityProject.publishFeePaid,
            _communityProject.lentAmount,
            _communityProject.interest,
            _communityProject.lastTimestamp
        );
    }

Affected source code:

Community.sol#L648

The text was updated successfully, but these errors were encountered:

code423n4 added bug Something isn't working G (Gas Optimization) labels Aug 6, 2022

code423n4 added a commit that referenced this issue Aug 6, 2022

0x1f8b issue #335

cf4a781

jack-the-pug added the valid label Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas Optimizations #335

Gas Optimizations #335

code423n4 commented Aug 6, 2022

Gas Optimizations #335

Gas Optimizations #335

Comments

code423n4 commented Aug 6, 2022

Gas

1. Is DebtToken needed?

2. Use Custom Errors instead of Revert Strings to save Gas

3. Remove None

4. ++i costs less gas compared to i++ or i += 1

5. There's no need to set default values for variables

6. Don't use storage variables for loops condition

7. Improve deserialization

8. Reorder conditionals to be cheaper

9. Save storage access

10. Reduce code size

11. Improve logic

12. Use memory instead storage

1. Is `DebtToken` needed?

4. `++i` costs less gas compared to `i++` or `i += 1`

12. Use `memory` instead `storage`