code-423n4 / 2022-08-rigor-findings

1 stars 0 forks source link

Gas Optimizations #398

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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;
parv3213 commented 2 years ago

Good suggestions!