Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #398

Open
code423n4 opened this issue Aug 6, 2022 · 1 comment
Open

Gas Optimizations #398

code423n4 opened this issue Aug 6, 2022 · 1 comment

Comments

@code423n4
Copy link
Contributor

1. SignatureDecoder.recoverKey() is called twice by two raiseDispute functions with the same result

Disputes' raiseDispute() is called only by Project's raiseDispute() with _data passed over:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L492-L502

    /// @inheritdoc IProject
    function raiseDispute(bytes calldata _data, bytes calldata _signature)
        external
        override
    {
        // Recover the signer from the signature
        address signer = SignatureDecoder.recoverKey(
            keccak256(_data),
            _signature,
            0
        );

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L534-L536

        // Make a call to Disputes contract raiseDisputes.
        IDisputes(disputes).raiseDispute(_data, _signature);
    }

Disputes' raiseDispute() repeats SignatureDecoder.recoverKey(keccak256(_data),_signature, 0) with the same result:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L84-L94

    function raiseDispute(bytes calldata _data, bytes calldata _signature)
        external
        override
        onlyProject
    {
        // Recover signer from signature
        address _signer = SignatureDecoder.recoverKey(
            keccak256(_data),
            _signature,
            0
        );

Recommended Mitigation Steps

Consider introducing the signer argument and sending the _signer to the downstream raiseDispute():

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L534-L536

        // Make a call to Disputes contract raiseDisputes.
-       IDisputes(disputes).raiseDispute(_data, _signature);
+       IDisputes(disputes).raiseDispute(_data, _signature, _signer);
    }

2. Unnecessary _msgSender calls in Community's lendToProject

lendToProject() does three calls instead of one:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L379-L380

        // Local instance of variable. For saving gas.
        address _sender = _msgSender();

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L442-L446

        // Transfer _lenderFee to HomeFi treasury from lender account
        _currency.safeTransferFrom(_msgSender(), homeFi.treasury(), _lenderFee);

        // Transfer _amountToProject to _project from lender account
        _currency.safeTransferFrom(_msgSender(), _project, _amountToProject);

Recommended Mitigation Steps

As _msgSender() is already saved to memory consider using _sender variable instead of the call

3. Unnecessary _msgSender calls in Project's inviteSC

inviteSC() does two calls instead of one:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L300-L304

        // Revert if sender is neither builder nor contractor.
        require(
            _msgSender() == builder || _msgSender() == contractor,
            "Project::!Builder||!GC"
        );

Same for acceptInviteSC():

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L322-L324

        for (uint256 i = 0; i < _length; i++) {
            tasks[_taskList[i]].acceptInvitation(_msgSender());
        }

Recommended Mitigation Steps

Consider introducing and using _sender memory variable instead of the calls

4. Unnecessary storage update

lentAmount is updated even if not changed, when _repayAmount <= _interest:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L785-L802

        if (_repayAmount > _interest) {
            // If repayment amount is greater than interest then
            // set lent = lent + interest - repayment.
            // And set interest = 0.
            uint256 _lentAndInterest = _lentAmount + _interest;

            // Revert if repayment amount is greater than sum of lent and interest.
            require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
            _interest = 0;
            _lentAmount = _lentAndInterest - _repayAmount;
        } else {
            // If repayment amount is lesser than interest, then set
            // interest = interest - repayment
            _interest -= _repayAmount;
        }

        // Update community  project details
        _communityProject.lentAmount = _lentAmount;

Recommended Mitigation Steps

Consider moving storage update to the part of logic where it happens:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L785-L802

        if (_repayAmount > _interest) {
            // If repayment amount is greater than interest then
            // set lent = lent + interest - repayment.
            // And set interest = 0.
            uint256 _lentAndInterest = _lentAmount + _interest;

            // Revert if repayment amount is greater than sum of lent and interest.
            require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
            _interest = 0;
            _lentAmount = _lentAndInterest - _repayAmount;
+           _communityProject.lentAmount = _lentAmount;
        } else {
            // If repayment amount is lesser than interest, then set
            // interest = interest - repayment
            _interest -= _repayAmount;
        }

        // Update community  project details
-       _communityProject.lentAmount = _lentAmount;
code423n4 added a commit that referenced this issue Aug 6, 2022
@parv3213
Copy link
Collaborator

parv3213 commented Aug 7, 2022

Good suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants