Nested Finance contest - _Adam's results

QA01 Uncapped Fees

There is nothing stopping a malicious owner changing both entryFees & exitFees to 100%. Recommend having an upper limit and using that instead of 10,000 in the 2 following require checks.

NestedFactory.sol#L161 NestedFactory.sol#L169

QA02 Incomplete Natspec

MixinOperatorResolver.sol#L50 - missing @return in natspec OwnableProxyDelegation.sol#L34 - missing @return address OwnableProxyDelegation.sol#L55 - missing @param newOwner BeefyZapBiswapLPVaultOperator.sol#L231 - missing @return mintedLpAmount BeefyZapBiswapLPVaultOperator.sol#L263 - missing @param reserveA, @param reserveB, @param router, @return swapAmount BeefyZapUniswapLPVaultOperator.sol#L231 - mising @return mintedLpAmount BeefyZapUniswapLPVaultOperator.sol#L262 - missing @param reserveA, @param reserveB, @param router, @return swapAmount

G01 Custom Errors

As your using a solidity version greater 0.8.4 you can replace revert strings with custom errors. This will save in deployment costs and runtime costs. I ran a test in remix comparing a revert string vs custom errors and found that 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)

There are 131 revert strings throughout the in scope files that can be replaced with custom errors:

G02 Long Revert Strings

If you opt not to use custom errors keeping revert strings <= 32 bytes in length will save gas. I ran a test in remix and found the savings for a single short revert string vs long string to be 9,377 gas in deployment cost and 18 gas on function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "short error message"); 
		(Deployment cost: 114,799, Cost on function call: 23,392)	
		vs 
		require(a != 0, "A longer Error Message over 32 bytes in     
        length"); 
		(Deployment cost: 124,176, Cost on function call: 23,410)	
	}
}

I recommend shortenning the following revert strings to <= 32 bytes in length: TimelockControllerEmergency.sol#L229-L230 TimelockControllerEmergency.sol#L243-L244 TimelockControllerEmergency.sol#L256 TimelockControllerEmergency.sol#L319-L320 TimelockControllerEmergency.sol#L334-L335 TimelockControllerEmergency.sol#L342 TimelockControllerEmergency.sol#L359 TimelockControllerEmergency.sol#L375

G03 Public Functions that can be External

The following functions are currently set to public but are not called anywhere in their contract and can be switched to external.

NestedFactory.sol#L114 OwnableProxyDelegation.sol#L50 OwnableProxyDelegation.sol#L56 OwnerProxy.sol#L16 TimelockControllerEmergency.sol#L199-L206 TimelockControllerEmergency.sol#L221-L228 TimelockControllerEmergency.sol#L255 TimelockControllerEmergency.sol#L274-L280 TimelockControllerEmergency.sol#L295-L299 TimelockControllerEmergency.sol#L312-L318

G04 For Loop Optimisations

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

Instances where unchecked and pre Increments can be implemented in for loops:

NestedFactory.sol#L124 NestedFactory.sol#L136 NestedFactory.sol#L196 NestedFactory.sol#L256 NestedFactory.sol#L315 NestedFactory.sol#L333 NestedFactory.sol#L369 NestedFactory.sol#L412 NestedFactory.sol#L651 OperatorResolver.sol#L40 OperatorResolver.sol#L60 OperatorResolver.sol#L75 MixinOperatorResolver.sol#L37 MixinOperatorResolver.sol#L56 BeefyVaultOperator.sol#L18 BeefyZapBiswapLPVaultOperator.sol#L27 BeefyZapUniswapLPVaultOperator.sol#L27 YearnCurveVaultOperator.sol#L42 CurveHelpers.sol#L22 CurveHelpers.sol#L42 CurveHelpers.sol#L62 CurveHelpers.sol#L86 TimelockControllerEmergency.sol#L84 TimelockControllerEmergency.sol#L89 TimelockControllerEmergency.sol#L234 TimelockControllerEmergency.sol#L324 OperatorScripts.sol#L67 OperatorScripts.sol#L80

G05 && in Require Functions

If optimising for running costs over deployment costs you can seperate && in require functions into 2 parts. I ran a basic test in remix and it cost an extra 234 gas to deploy but will save ~9 gas everytime the require function is called.

contract Test {
	uint256 a = 0;
	uint256 b = 1;

	function test() external {
		require(a == 0 && b > a) 
		(Deployment cost: 123,291, Cost on function call: 29,371)
		vs
		require(a == 0);
		require(b > a);
		(Deployment cost: 123,525, Cost on function call: 29,362)
	}
}

BeefyVaultOperator.sol#L54 BeefyZapBiswapLPVaultOperator.sol#L64-L65 BeefyZapUniswapLPVaultOperator.sol#L64-L65 ParaswapOperator.sol#L16