Rigor Protocol contest - _Adam's results

[L01] index[1] never used for community.members[i]

Description: In the function createCommunity communities.members[i] is set with i being the 0. In addMember communities.members[i] is set using the variable memberCount after it has been updated. As memberCount will already equal 1 the first time the function addMember will be called index 2 will be the first position a new member will be added.

LOC: Community.sol#L146-L147 Community.sol#L198-L199

Recommendation: Switch line 118 and 117 so that members is added before the increment of memberCount.

[L02] 2 Step Admin Changes

Description Critical changes such as replacing the admin should be a 2 step process to protect against human error. While the errors are unlikely important parts of the contract would become unusable if they occured.

LOC: HomeFi.sol#L157-L168

Recommendation: Consider changing the replaceAdmin function to be a 2 step procedure where the new admin must accept the admin role before the previous admin loses their privileges.

[L03] Use of safeMint over mint()

Description: As per openZepplin safeMint() should be used instead of mint() to prevent the loss of NFT's if the receiver is a contract account that hasn't implemented ERC721Receiver.

LOC: HomeFi.sol#L292

Recommendation: Replace mint() with safeMint().

[N01] Typos

LOC: Project.sol#L514 - Change Revet to Revert Project.sol#L520 - Change Revet to Revert Community.sol#L849 - Change Burn to Mint

[G01] Repeated Use of State Variable projectCount

Description: The state variable projectCount is referenced 3 times without ever being modified. 3 SLOAD's can be saved by saving the return value of mintNFT() on L225 to a local variable and using that instead.

LOC: HomeFi.sol#L225-L231

[G02] Packing State Variables

Description: The state variable contractorDelegated can be moved up to line 70 to be packed into 1 storage slot with contractor & contractorConfirmed. This will save 1 storage slot (~20,000 gas).

LOC: Project.sol#L78

[G03] Custom Errors

Description: As your using a solidity version > 0.8.4 you can replace revert strings with custom errors. This will save in deployment costs and runtime costs. Based on a test in remix, replacing a single revert string with a custom error saved 12,404 gas in deployment cost and 86 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "check failed");
	}
}   (Deployment cost: 114,703, Cost on Function call: 23,392)
vs 
contract Test {
	uint256 a;
	error checkFailed();
	function check() external {
		if (a != 0) revert checkFailed();
	}
}   (Deployment cost: 102,299, Cost on Function call: 23,306)

LOC: Their are 74 instances of revert strings throughout the Rigor contracts that can be replaced with custom errors.

[G04] For Loop Optimisations

Description: When incrementing i in for loops there is no chance of overflow so unchecked can be used to save gas. I ran a simple test in remix and found deployment savings of 31,653 gas and on each function call saved ~141 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; ++i) {
		Deployment Cost: 125,637, Cost on function call: 24,601
		vs
		for (uint256 i; i < 1; ) {
		// for loop body
		unchecked { ++i; }
		Deployment Cost: 93,984, Cost on function call: 24,460
		}
	}
}

In for loops pre increments can also be used to save a small amount of gas per iteration. I ran a test in remix using a for loop and found the deployment savings of 497 gas and ~5 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; i++) {
		(Deployment cost: 118,408, Cost on function call: 24,532)
		vs
		for (uint256 i; i < 1; ++i) {
		(Deployment cost: 117,911, Cost on function call: 24,527)
		}
	}
}

It is also recommended not to loop over state variables as an SLOAD will need to be done every loop. Recommend caching to memory and looping over that instead.

LOC: Community.sol#L624-L626 HomeFiProxy.sol#L87-L89 HomeFiProxy.sol#L136-L138 Project.sol#L248-L257 Project.sol#L311-L313 Project.sol#L322-L324 Project.sol#L368-L370 Project.sol#L603-L631 Project.sol#L650-L678 Project.sol#L710-L712 Tasks.sol#L181

[G05] x = x + y is Cheaper than x += y

Description: Based on test in remix you can save ~1,007 gas on deployment and ~15 gas on execution cost if you use x = x + y over x += y. (Is only true for storage variables)

contract Test {
	uint256 x = 1;
	function test() external {
		x += 3; 
		(Deployment Cost: 153,124, Execution Cost: 30,369)
		vs
		x = x + 1;
		(Deployment Cost: 152,117, Execution Cost: 30,354)
	}

}

LOC: Community.sol#L423 Community.sol#L433 HomeFi.sol#L289 Project.sol#L179 Project.sol#L290 Project.sol#L440