Golom contest - _Adam's results

Judge has assessed an item in Issue #563 as Medium risk. The relevant finding follows:

[L02] payEther() can fail if sending to non EOA

Description: As transfer() has a limit of 2,300 gas, when transfering eth to smart contract accounts the transfer can fail. This can be due to either failure to implement a payable fallback function or the function call costs exceed the limit of 2300 gas.

LOC: GolomTrader.sol#L151-L156

Recommendation: As payEther is only called from within contracts with reentrancy guard It can be replaced with call().

Judge has assessed an item in Issue #563 as Medium risk. The relevant finding follows:

[L03] Use of transferFrom instead of safeTransferFrom

Description: When using transferFrom if the receiving address is a smart contract that hasn't correctly implemented a way to receive ERC721 tokens then the NFT can be lost.

LOC: GolomTrader.sol#L236 GolomTrader.sol#L301 GolomTrader.sol#L361

Recommendation: Consider using the safeTransferFrom function instead of transferFrom.

[L01] Floating Pragma

Description: Non Library/Interface contracts should be deployed with a locked pragma version. This prevents the contract being deployed with a version that wasn't thoroughly tested against in development.

LOC: GolomToken.sol#L2

Recommendation: Lock the pragma to 0.8.11 to be consistent with other golom contracts.

[L02] payEther() can fail if sending to non EOA

Description: As transfer() has a limit of 2,300 gas, when transfering eth to smart contract accounts the transfer can fail. This can be due to either failure to implement a payable fallback function or the function call costs exceed the limit of 2300 gas.

LOC: GolomTrader.sol#L151-L156

Recommendation: As payEther is only called from within contracts with reentrancy guard It can be replaced with call().

[L03] Use of transferFrom instead of safeTransferFrom

Description: When using transferFrom if the receiving address is a smart contract that hasn't correctly implemented a way to receive ERC721 tokens then the NFT can be lost.

LOC: GolomTrader.sol#L236 GolomTrader.sol#L301 GolomTrader.sol#L361

Recommendation: Consider using the safeTransferFrom function instead of transferFrom.

[L04] Incompatible with Non ERC721 NFT's

Description: NFT's such as cryptopunks that don't adhere to the ERC721 standards will always fail when the protocol attempts to transfer them.

LOC: GolomTrader.sol#L236 GolomTrader.sol#L301 GolomTrader.sol#L361

Recommendation: If choosing to support non ERC721 compliant NFT's I recommending adding custom wrappers similar to NFTX's Implementation for cryptopunks.

[N01] Incomplete Natspec

Description: Natspec is missing in the following locations:

LOC: VoteEscrowCore.sol#L404 - missing @return VoteEscrowCore.sol#L975 - missing @param _ tokenId VoteEscrowCore.sol#L989 - missing @param _ tokenId VoteEscrowCore.sol#L1005 - missing @param _ tokenId VoteEscrowDelegation.sol#L68 - @notice is incorrect GolomTrader.sol#L331 - missing @param tokenId & @param proof

[N02] Remove Old Code

Description: Recommend removing the old commented out code before deployment.

LOC: VoteEscrowDelegation.sol#L6 VoteEscrowDelegation.sol#L218-L225

[G01] Custom Errors

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: GolomToken.sol#L24 GolomToken.sol#L43 GolomToken.sol#L51 GolomToken.sol#L69 VoteEscrowCore.sol#L538 VoteEscrowCore.sol#L894 VoteEscrowCore.sol#L928-L929 VoteEscrowCore.sol#L945-L946 VoteEscrowCore.sol#L982-L983 VoteEscrowCore.sol#L996-L999 VoteEscrowCore.sol#L1008-L1011 VoteEscrowCore.sol#L1082 VoteEscrowCore.sol#L1227 VoteEscrowDelegation.sol#L72-L73 VoteEscrowDelegation.sol#L99 VoteEscrowDelegation.sol#L130 VoteEscrowDelegation.sol#L186 VoteEscrowDelegation.sol#L211 VoteEscrowDelegation.sol#L239 RewardDistributor.sol#L173 RewardDistributor.sol#L181-L185 RewardDistributor.sol#L220 RewardDistributor.sol#L292 RewardDistributor.sol#L309 GolomTrader.sol#L177 GolomTrader.sol#L211 GolomTrader.sol#L217 GolomTrader.sol#L222 GolomTrader.sol#L226-L227 GolomTrader.sol#L235 GolomTrader.sol#L299 GolomTrader.sol#L359 GolomTrader.sol#L455

[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)	
	}
}

LOC: GolomToken.sol#L24 VoteEscrowDelegation.sol#L73 RewardDistributor.sol#L181 RewardDistributor.sol#L292 RewardDistributor.sol#L309

[G03] Initialising State Variables to Default Values

When initialising a state variable to its default variable, it is cheaper to leave blank. I ran a test in remix that initialises a single variable and got a saving of 2,246 gas.

contract Test {
    uint256 public variable = 0;     (69,312 gas)
    vs
    uint256 public variable;         (67,066 gas)
}

LOC: VoteEscrowDelegation.sol#L50 RewardDistributor.sol#L45

[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 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: VoteEscrowCore.sol#L745 VoteEscrowCore.sol#L1044 VoteEscrowCore.sol#L1115 VoteEscrowCore.sol#L1167 VoteEscrowDelegation.sol#L171 VoteEscrowDelegation.sol#L189 VoteEscrowDelegation.sol#L199 - recommend cacheing array.length RewardDistributor.sol#L143 RewardDistributor.sol#L157 RewardDistributor.sol#L180-L183 RewardDistributor.sol#L226 - recommend cacheing epoch RewardDistributor.sol#L258 - recommend cacheing epoch RewardDistributor.sol#L273 - recommend cacheing epoch GolomTrader.sol#L415

[G05] State Variables that can be Immutable

State variables that are initialised in the constructor and then never updated anywhere can be changed to immutable. Based on the following test in remix switching to immutable variables can save 26,376 in deployment costs and 2,456 whenever referencing the variable.

contract Test {
	address public supply; 
	(Deployment Cost: 167,940, Cost on function call: 26,861)
	vs
	address public immutable supply;
	(Deployment Cost: 141,564, Cost on function call: 24,405)

	constructor(address _supply) {
		supply = _supply;
	} 

	function test() external {
		address testAddress = supply; // to test referencing gas costs
	}
}

LOC: RewardDistributor.sol#L46 - startTime can be changed to immutable

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

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 state 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: VoteEscrowCore.sol#L499 VoteEscrowCore.sol#L512