[Aikido] Fix security issue in elliptic via minor version upgrade from 6.5.4 to 6.6.1

[aikido-autofix]

This pull request addresses identified vulnerabilities and implements the necessary fixes to strengthen our security posture. Please review and approve so we can merge these changes promptly and reduce potential risk.

Thanks , The security team.

This PR will resolve the following CVEs:

CVE ID	Severity	Description
CVE-2024-42461	🚨 CRITICAL	In the Elliptic package 6.5.6 for Node.js, ECDSA signature malleability occurs because BER-encoded signatures are allowed.
CVE-2024-48949	🚨 CRITICAL	The verify function in lib/elliptic/eddsa/index.js in the Elliptic package before 6.5.6 for Node.js omits "sig.S().gte(sig.eddsa.curve.n)
GHSA-vjh7-7g9h-fjfh	🚨 CRITICAL	### Summary Private key can be extracted from ECDSA signature upon signing a malformed input (e.g. a string or a number), which could e.g. come from JSON network input Note that elliptic by design accepts hex strings as one of the possible input types ### Details In this code: https://github.c...
CVE-2024-42459	MEDIUM	In the Elliptic package 6.5.6 for Node.js, EDDSA signature malleability occurs because there is a missing signature length check, and thus zero-valued bytes can be removed or appended.
CVE-2024-42460	MEDIUM	In the Elliptic package 6.5.6 for Node.js, ECDSA signature malleability occurs because there is a missing check for whether the leading bit of r and s is zero.
CVE-2024-48948	MEDIUM	The Elliptic package 6.5.7 for Node.js, in its for ECDSA implementation, does not correctly verify valid signatures if the hash contains at least four leading 0 bytes and when the order of the elliptic curve's base point is smaller than the hash, because of an _truncateToN anomaly. This leads to val...

Related Tasks:

• https://github.com/vechain/security/issues/229

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

• arbitrary-send-eth (1 results) (High)
• calls-loop (3 results) (Low)
• reentrancy-events (3 results) (Low)
• timestamp (3 results) (Low)

arbitrary-send-eth

Impact: High
Confidence: Medium

• ID-0
  SimpleAccount._call(address,uint256,bytes) sends eth to arbitrary user
  Dangerous calls:
  • (success,result) = target.call{value: value}(data)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 484 to 491 in 7d0f9f2

	function _call(address target, uint256 value, bytes memory data) internal {
	(bool success, bytes memory result) = target.call{value: value}(data);
	if (!success) {
	assembly {
	revert(add(result, 32), mload(result))
	}
	}
	}

calls-loop

Impact: Low
Confidence: Medium

• ID-1
  SimpleAccount._call(address,uint256,bytes) has external calls inside a loop: (success,result) = target.call{value: value}(data)
  Calls stack containing the loop:
  SimpleAccount.executeBatchWithCustomAuthorization(address[],uint256[],bytes[],uint256,uint256,bytes32,bytes)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 484 to 491 in 7d0f9f2

	function _call(address target, uint256 value, bytes memory data) internal {
	(bool success, bytes memory result) = target.call{value: value}(data);
	if (!success) {
	assembly {
	revert(add(result, 32), mload(result))
	}
	}
	}

• ID-2
  SimpleAccount._call(address,uint256,bytes) has external calls inside a loop: (success,result) = target.call{value: value}(data)
  Calls stack containing the loop:
  SimpleAccount.executeBatchWithAuthorization(address[],uint256[],bytes[],uint256,uint256,bytes32,bytes)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 484 to 491 in 7d0f9f2

	function _call(address target, uint256 value, bytes memory data) internal {
	(bool success, bytes memory result) = target.call{value: value}(data);
	if (!success) {
	assembly {
	revert(add(result, 32), mload(result))
	}
	}
	}

• ID-3
  SimpleAccount._call(address,uint256,bytes) has external calls inside a loop: (success,result) = target.call{value: value}(data)
  Calls stack containing the loop:
  SimpleAccount.executeBatch(address[],uint256[],bytes[])

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 484 to 491 in 7d0f9f2

	function _call(address target, uint256 value, bytes memory data) internal {
	(bool success, bytes memory result) = target.call{value: value}(data);
	if (!success) {
	assembly {
	revert(add(result, 32), mload(result))
	}
	}
	}

reentrancy-events

Impact: Low
Confidence: Medium

• ID-4
  Reentrancy in SimpleAccountFactory.createAccountWithSalt(address,uint256):
  External calls:
  • createdAccount = SimpleAccount(address(new ERC1967Proxy(implementationToUse,abi.encodeCall(SimpleAccount.initialize,(owner)))))
    Event emitted after the call(s):
  • AccountCreated(createdAccount,owner,salt)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccountFactory.sol

Lines 199 to 259 in 7d0f9f2

	function createAccountWithSalt(
	address owner,
	uint256 salt
	) public returns (SimpleAccount createdAccount) {
	// Calculate address with V1 implementation first (same as getAccountAddressWithSalt)
	address addressGeneratedWithV1 = Create2.computeAddress(
	bytes32(salt),
	keccak256(
	abi.encodePacked(
	type(ERC1967Proxy).creationCode,
	abi.encode(
	address(accountImplementationV1),
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	)
	);
	// We also calculate the address with the V3 implementation (in case the account is V3)
	address addressGeneratedWithV3 = Create2.computeAddress(
	bytes32(salt),
	keccak256(
	abi.encodePacked(
	type(ERC1967Proxy).creationCode,
	abi.encode(
	address(accountImplementationV3),
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	)
	);
	// We check if the account is legacy or not
	bool mustUseV1 = _mustUseV1Implementation(addressGeneratedWithV1);
	// Let's check if the account is already deployed (and we use V1 address or V3 address based on the legacy check)
	address simpleAccountAddress = mustUseV1
	? address(addressGeneratedWithV1)
	: address(addressGeneratedWithV3);
	// Check if account already exists
	if (simpleAccountAddress.code.length > 0) {
	return SimpleAccount(payable(simpleAccountAddress));
	}
	// For legacy accounts, deploy with V1 implementation
	address implementationToUse = mustUseV1
	? address(accountImplementationV1)
	: address(accountImplementationV3);
	createdAccount = SimpleAccount(
	payable(
	new ERC1967Proxy{salt: bytes32(salt)}(
	implementationToUse,
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	);
	emit AccountCreated(createdAccount, owner, salt);
	}

• ID-5
  Reentrancy in SimpleAccountFactory.createAccountWithVersion(address,uint256):
  External calls:
  • createdAccount = SimpleAccount(address(new ERC1967Proxy(implementation,abi.encodeCall(SimpleAccount.initialize,(owner)))))
    Event emitted after the call(s):
  • AccountCreated(createdAccount,owner,salt)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccountFactory.sol

Lines 271 to 320 in 7d0f9f2

	function createAccountWithVersion(
	address owner,
	uint256 _version
	)
	public
	onlyRole(DEFAULT_ADMIN_ROLE)
	returns (SimpleAccount createdAccount)
	{
	require(
	_version == 1 || _version == 3,
	"Only versions 1 and 3 are supported"
	);
	uint256 salt = uint256(uint160(owner));
	// Calculate address with the specified implementation
	address implementation = _version == 1
	? address(accountImplementationV1)
	: address(accountImplementationV3);
	address accountAddress = Create2.computeAddress(
	bytes32(salt),
	keccak256(
	abi.encodePacked(
	type(ERC1967Proxy).creationCode,
	abi.encode(
	implementation,
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	)
	);
	// Check if account already exists
	if (accountAddress.code.length > 0) {
	return SimpleAccount(payable(accountAddress));
	}
	// Deploy with specified implementation
	createdAccount = SimpleAccount(
	payable(
	new ERC1967Proxy{salt: bytes32(salt)}(
	implementation,
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	);
	emit AccountCreated(createdAccount, owner, salt);
	}

• ID-6
  Reentrancy in SimpleAccountFactory.createAccount(address):
  External calls:
  • createdAccount = SimpleAccount(address(new ERC1967Proxy(implementationToUse,abi.encodeCall(SimpleAccount.initialize,(owner)))))
    Event emitted after the call(s):
  • AccountCreated(createdAccount,owner,salt)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccountFactory.sol

Lines 128 to 189 in 7d0f9f2

	function createAccount(
	address owner
	) public returns (SimpleAccount createdAccount) {
	uint256 salt = uint256(uint160(owner));
	// Calculate address with V1 implementation first (same as getAccountAddress)
	address addressGeneratedWithV1 = Create2.computeAddress(
	bytes32(salt),
	keccak256(
	abi.encodePacked(
	type(ERC1967Proxy).creationCode,
	abi.encode(
	address(accountImplementationV1),
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	)
	);
	// We also calculate the address with the V3 implementation (in case the account is V3)
	address addressGeneratedWithV3 = Create2.computeAddress(
	bytes32(salt),
	keccak256(
	abi.encodePacked(
	type(ERC1967Proxy).creationCode,
	abi.encode(
	address(accountImplementationV3),
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	)
	);
	// We check if the account is legacy or not
	bool mustUseV1 = _mustUseV1Implementation(addressGeneratedWithV1);
	// Let's check if the account is already deployed (and we use V1 address or V3 address based on the legacy check)
	address simpleAccountAddress = mustUseV1
	? address(addressGeneratedWithV1)
	: address(addressGeneratedWithV3);
	// Check if account already exists
	if (simpleAccountAddress.code.length > 0) {
	return SimpleAccount(payable(simpleAccountAddress));
	}
	// If the account is legacy, we use the V1 implementation address, otherwise we use the V3 implementation address
	address implementationToUse = mustUseV1
	? address(accountImplementationV1)
	: address(accountImplementationV3);
	createdAccount = SimpleAccount(
	payable(
	new ERC1967Proxy{salt: bytes32(salt)}(
	implementationToUse,
	abi.encodeCall(SimpleAccount.initialize, (owner))
	)
	)
	);
	emit AccountCreated(createdAccount, owner, salt);
	}

timestamp

Impact: Low
Confidence: Medium

• ID-7
  SimpleAccount.executeBatchWithAuthorization(address[],uint256[],bytes[],uint256,uint256,bytes32,bytes) uses timestamp for comparisons
  Dangerous comparisons:
  • require(bool,string)(block.timestamp > validAfter,Authorization not yet valid)
  • require(bool,string)(block.timestamp < validBefore,Authorization expired)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 206 to 248 in 7d0f9f2

	function executeBatchWithAuthorization(
	address[] calldata to,
	uint256[] calldata value,
	bytes[] calldata data,
	uint256 validAfter,
	uint256 validBefore,
	bytes32 nonce,
	bytes calldata signature
	) external payable {
	// Check array lengths match
	require(
	to.length == value.length && value.length == data.length,
	"Array lengths mismatch"
	);
	// Check that the signature is not used yet
	require(
	!usedNonces[nonce],
	"Nonce already used, please sign a new transaction"
	);
	// Check time validity for all transactions
	require(block.timestamp > validAfter, "Authorization not yet valid");
	require(block.timestamp < validBefore, "Authorization expired");
	// Validate batch authorization
	_validateBatchAuthorization(
	to,
	value,
	data,
	validAfter,
	validBefore,
	nonce,
	signature
	);
	usedNonces[nonce] = true;
	// Execute each transaction
	for (uint256 i = 0; i < to.length; i++) {
	_call(to[i], value[i], data[i]);
	}
	}

• ID-8
  SimpleAccount._validateAuthorization(address,uint256,bytes,uint256,uint256,bytes) uses timestamp for comparisons
  Dangerous comparisons:
  • require(bool,string)(block.timestamp > validAfter,Authorization not yet valid)
  • require(bool,string)(block.timestamp < validBefore,Authorization expired)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 374 to 401 in 7d0f9f2

	function _validateAuthorization(
	address to,
	uint256 value,
	bytes calldata data,
	uint256 validAfter,
	uint256 validBefore,
	bytes calldata signature
	) internal view {
	require(block.timestamp > validAfter, "Authorization not yet valid");
	require(block.timestamp < validBefore, "Authorization expired");
	bytes32 structHash = keccak256(
	abi.encode(
	keccak256(
	"ExecuteWithAuthorization(address to,uint256 value,bytes data,uint256 validAfter,uint256 validBefore)"
	),
	to,
	value,
	keccak256(data),
	validAfter,
	validBefore
	)
	);
	bytes32 digest = _hashTypedDataV4(structHash);
	address recoveredAddress = ECDSA.recover(digest, signature);
	require(recoveredAddress == owner, "Invalid signer");
	}

• ID-9
  SimpleAccount.executeBatchWithCustomAuthorization(address[],uint256[],bytes[],uint256,uint256,bytes32,bytes) uses timestamp for comparisons
  Dangerous comparisons:
  • require(bool,string)(block.timestamp > validAfter,Authorization not yet valid)
  • require(bool,string)(block.timestamp < validBefore,Authorization expired)

smart-accounts/packages/contracts/contracts/accounts/SimpleAccount.sol

Lines 257 to 298 in 7d0f9f2

	function executeBatchWithCustomAuthorization(
	address[] calldata to,
	uint256[] calldata value,
	bytes[] calldata data,
	uint256 validAfter,
	uint256 validBefore,
	bytes32 nonce,
	bytes calldata signature
	) external payable {
	// Check array lengths match
	require(
	to.length == value.length && value.length == data.length,
	"Array lengths mismatch"
	);
	// Check that the signature is not used yet
	require(
	!usedNonces[nonce],
	"Nonce already used, please sign a new transaction"
	);
	// Check time validity for all transactions
	require(block.timestamp > validAfter, "Authorization not yet valid");
	require(block.timestamp < validBefore, "Authorization expired");
	_validateBatchTransactionWithCustomDomain(
	to,
	value,
	data,
	validAfter,
	validBefore,
	nonce,
	signature
	);
	usedNonces[nonce] = true;
	// Execute each transaction
	for (uint256 i = 0; i < to.length; i++) {
	_call(to[i], value[i], data[i]);
	}
	}