Ambire invitational contest Automatic Findings

Report.md

Report

Gas Optimizations

	Issue	Instances
GAS-1	Cache array length outside of loop	3
GAS-2	For Operations that will not overflow, you could use unchecked	36
GAS-3	Don't initialize variables with default value	5
GAS-4	++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)	5

[GAS-1] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (3):

File: AmbireAccount.sol

164: 				for (uint256 i = 0; i < recoveryInfo.keys.length; i++) {

198: 		for (uint256 i = 0; i != toExec.length; i++) execute(toExec[i].txns, toExec[i].signature);

File: libs/SignatureValidator.sol

87: 			for (uint256 i = 0; i != signatures.length; i++) {

[GAS-2] For Operations that will not overflow, you could use unchecked

Instances (36):

File: AmbireAccount.sol

4: import './libs/SignatureValidator.sol';

4: import './libs/SignatureValidator.sol';

19: 	event LogErr(address indexed to, uint256 value, bytes data, bytes returnData); // only used in tryCatch

19: 	event LogErr(address indexed to, uint256 value, bytes data, bytes returnData); // only used in tryCatch

60: 		for (uint256 i = 0; i < len; i++) {

60: 		for (uint256 i = 0; i < len; i++) {

142: 		uint8 sigMode = uint8(signature[signature.length - 1]);

164: 				for (uint256 i = 0; i < recoveryInfo.keys.length; i++) {

164: 				for (uint256 i = 0; i < recoveryInfo.keys.length; i++) {

175: 					scheduledRecoveries[hash] = block.timestamp + recoveryInfo.timelock;

189: 		nonce = currentNonce + 1;

198: 		for (uint256 i = 0; i != toExec.length; i++) execute(toExec[i].txns, toExec[i].signature);

198: 		for (uint256 i = 0; i != toExec.length; i++) execute(toExec[i].txns, toExec[i].signature);

219: 		for (uint256 i = 0; i < len; i++) {

219: 		for (uint256 i = 0; i < len; i++) {

254: 			interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).

254: 			interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).

254: 			interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).

255: 			interfaceID == 0x4e2312e0; // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).

255: 			interfaceID == 0x4e2312e0; // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).

255: 			interfaceID == 0x4e2312e0; // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).

File: AmbireAccountFactory.sol

4: import './AmbireAccount.sol';

File: libs/Bytes.sol

14: 	|__________________________________*/

14: 	|__________________________________*/

24: 		index += 32;

File: libs/SignatureValidator.sol

4: import './Bytes.sol';

32: 			modeRaw = uint8(sig[sig.length - 1]);

34: 		sig.trimToSize(sig.length - 1);

46: 			modeRaw = uint8(sig[sig.length - 1]);

69: 			sig.trimToSize(sig.length - 1);

72: 			bytes32 sp = bytes32(Q - mulmod(uint256(s), uint256(px), Q));

73: 			bytes32 ep = bytes32(Q - mulmod(uint256(e), uint256(px), Q));

84: 			sig.trimToSize(sig.length - 1);

87: 			for (uint256 i = 0; i != signatures.length; i++) {

87: 			for (uint256 i = 0; i != signatures.length; i++) {

99: 				newLen = sig.length - 33;

[GAS-3] Don't initialize variables with default value

Instances (5):

File: AmbireAccount.sol

60: 		for (uint256 i = 0; i < len; i++) {

164: 				for (uint256 i = 0; i < recoveryInfo.keys.length; i++) {

198: 		for (uint256 i = 0; i != toExec.length; i++) execute(toExec[i].txns, toExec[i].signature);

219: 		for (uint256 i = 0; i < len; i++) {

File: libs/SignatureValidator.sol

87: 			for (uint256 i = 0; i != signatures.length; i++) {

[GAS-4] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (5):

File: AmbireAccount.sol

60: 		for (uint256 i = 0; i < len; i++) {

164: 				for (uint256 i = 0; i < recoveryInfo.keys.length; i++) {

198: 		for (uint256 i = 0; i != toExec.length; i++) execute(toExec[i].txns, toExec[i].signature);

219: 		for (uint256 i = 0; i < len; i++) {

File: libs/SignatureValidator.sol

87: 			for (uint256 i = 0; i != signatures.length; i++) {

Non Critical Issues

	Issue	Instances
NC-1	Constants should be defined rather than using magic numbers	2

[NC-1] Constants should be defined rather than using magic numbers

Instances (2):

File: libs/SignatureValidator.sol

113: 			require(tx.origin == address(1) || tx.origin == address(6969), 'SV_SPOOF_ORIGIN');

117: 			require(ecrecover(0, 0, 0, 0) != address(6969));

Low Issues

	Issue	Instances
L-1	abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()	4
L-2	Empty Function Body - Consider commenting why	1

[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

Instances (4):

File: AmbireAccountFactory.sol

46: 			uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)))))

File: libs/SignatureValidator.sol

58: 			if (mode == SignatureMode.EthSign) hash = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash));

81: 			require(e == keccak256(abi.encodePacked(R, uint8(parity), px, hash)), 'SV_SCHNORR_FAILED');

89: 					uint160(uint256(keccak256(abi.encodePacked(signer, recoverAddrImpl(hash, signatures[i], false)))))

[L-2] Empty Function Body - Consider commenting why

Instances (1):

File: AmbireAccount.sol

68: 	receive() external payable {}

High Issues

	Issue	Instances
H-1	Using delegatecall inside a loop	1

[H-1] Using delegatecall inside a loop

Impact:

When calling delegatecall the same msg.value amount will be accredited multiple times.

Instances (1):

File: AmbireAccount.sol

60: 		for (uint256 i = 0; i < len; i++) {