Rigor Protocol contest - Sm4rty's results

1. Consider two-phase ownership transfer

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

Instances:

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol#L150-L157

    function changeProxyAdminOwner(address _newAdmin)
        external
        onlyOwner
        nonZero(_newAdmin)
    {
        // Transfer ownership to new admin.
        proxyAdmin.transferOwnership(_newAdmin);
    }

References:

https://code4rena.com/reports/2022-05-sturdy#n-13-consider-two-phase-ownership-transfer

---

2. Multiple initialization due to initialize function not having initializer modifier.

Description

The attacker can initialize the contract, take malicious actions, and allow it to be re-initialized by the project without any error being noticed.

Instances

Tasks.sol#L7

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

1. 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)

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).

Instances:

HomeFi.sol

HomeFi.sol:73:        require(admin == _msgSender(), "HomeFi::!Admin");
HomeFi.sol:78:        require(_address != address(0), "HomeFi::0 address");
HomeFi.sol:84:        require(_oldAddress != _newAddress, "HomeFi::!Change");
HomeFi.sol:142:        require(!addrSet, "HomeFi::Set");
HomeFi.sol:191:        require(lenderFee != _newLenderFee, "HomeFi::!Change");

Project.sol

Project.sol:123:        require(!contractorConfirmed, "Project::GC accepted");
Project.sol:132:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:135:        require(_contractor != address(0), "Project::0 address");
Project.sol:150:        require(_msgSender() == builder, "Project::!B");
Project.sol:153:        require(contractor != address(0), "Project::0 address");
Project.sol:176:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:195:        require(_cost > 0, "Project::!value>0");
Project.sol:238:        require(_taskCount == taskCount, "Project::!taskCount");
Project.sol:241:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:245:        require(_length == _taskCosts.length, "Project::Lengths !match");
Project.sol:277:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:308:        require(_length == _scList.length, "Project::Lengths !match");
Project.sol:341:        require(_projectAddress == address(this), "Project::!Project");
Project.sol:369:        require(tasks[_taskID].getState() == 3, "Project::!Complete");
Project.sol:406:        require(_project == address(this), "Project::!projectAddress");
Project.sol:511:        require(_project == address(this), "Project::!projectAddress");
Project.sol:530:        require(getAlerts(_task)[2], "Project::!SCConfirmed");
Project.sol:753:        require(_sc != address(0), "Project::0 address");

Disputes.sol

Disputes.sol:39:        require(_address != address(0), "Disputes::0 address");
Disputes.sol:46:        require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Disputes.sol:52:        require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Disputes.sol:183:        require(_result, "Disputes::!Member");

DebtToken.sol:50

DebtToken.sol:50:        require(_communityContract != address(0), "DebtToken::0 address");

ProjectFactory.sol

ProjectFactory.sol:36:        require(_address != address(0), "PF::0 address");
ProjectFactory.sol:84:        require(_msgSender() == homeFi, "PF::!HomeFiContract");

libraries/Tasks.sol

libraries/Tasks.sol:44:        require(_self.state == TaskStatus.Inactive, "Task::active");
libraries/Tasks.sol:50:        require(_self.state == TaskStatus.Active, "Task::!Active");
libraries/Tasks.sol:124:        require(_self.subcontractor == _sc, "Task::!SC");

Community.sol

Community.sol:69:        require(_address != address(0), "Community::0 address");
Community.sol:75:        require(_msgSender() == homeFi.admin(), "Community::!admin");
Community.sol:241:        require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Community.sol:248:        require(_community.isMember[_builder], "Community::!Member");
Community.sol:536:        require(_builder == _projectInstance.builder(), "Community::!Builder");
Community.sol:557:        require(!restrictedToAdmin, "Community::restricted");
Community.sol:568:        require(restrictedToAdmin, "Community::!restricted");
Community.sol:764:        require(_repayAmount > 0, "Community::!repay");
Community.sol:792:        require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
HomeFiProxy.sol:41:       require(_address != address(0), "Proxy::0 address");

HomeFiProxy.sol

HomeFiProxy.sol:81:        require(_length == _implementations.length, "Proxy::Lengths !match");
HomeFiProxy.sol:133:        require(_length == _contractAddresses.length, "Proxy::Lengths !match");

References:

https://blog.soliditylang.org/2021/04/21/custom-errors/

Remediation:

I suggest replacing revert strings with custom errors.

---

2. !=0 instead of >0 for UINT

0 is less efficient than != 0 for unsigned integers (with proof) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

Instances

Project.sol:195 Community.sol:764

Project.sol:195:        require(_cost > 0, "Project::!value>0");
Community.sol:764:        require(_repayAmount > 0, "Community::!repay");

Reference:

https://twitter.com/gzeon/status/1485428085885640706

Remediation:

I suggest changing > 0 with != 0. Also, please enable the Optimizer.

---

3. Preincrement costs less gas as compared to Postincrement :

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)

i++ increments i and returns 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 , no need for temporary variable here

In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.

Instances:

Project.sol:625 Project.sol:672 Community.sol:140

Project.sol:625:                    _loopCount++;
Project.sol:672:                    _loopCount++;
Community.sol:140:        communityCount++;

---

4. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0; or bool abc; instead of bool abc = false

Instances:

Project.sol:248 Project.sol:311 Project.sol:322 libraries/Tasks.sol:181 Community.sol:624 HomeFiProxy.sol:87

// uint
Project.sol:248:        for (uint256 i = 0; i < _length; i++) {
Project.sol:311:        for (uint256 i = 0; i < _length; i++) {
Project.sol:322:        for (uint256 i = 0; i < _length; i++) {
libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
Community.sol:624:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) 

Project.sol:412

//bool
Project.sol:412:        bool _unapproved = false;

---