Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Nexus.sol is not compatible with eip-712

bauchibred

Summary

Nexus.sol is not compatible with eip-712

Vulnerability Details

The below snippets are how the signatures are created/expected, see https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/Nexus.sol#L327-L341

/// @notice Wrapper around `_eip712Hash()` to produce a replay-safe hash fron the given `hash`.
///
/// @dev The returned EIP-712 compliant replay-safe hash is the result of:
/// keccak256(
/// \x19\x01 ||
/// this.domainSeparator ||
/// hashStruct(BiconomyNexusMessage({ hash: `hash`}))
/// )
///
/// @param hash The original hash.
///
/// @return The corresponding replay-safe hash.
function replaySafeHash(bytes32 hash) public view virtual returns (bytes32) {
return _eip712Hash(hash);
}

And https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/Nexus.sol#L367-L370

function _eip712Hash(bytes32 hash) internal view virtual returns (bytes32) {
return keccak256(abi.encodePacked("\x19\x01", _domainSeparator(), keccak256(abi.encode(_MESSAGE_TYPEHASH, hash))));
}
​

Issue however is that this is not compatible with EIP712, because from EIP712:

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

However in our case the construction of the signatures are done directly with the hash instead of first applying a keccak hash to the procedure.

As a result, a signature generated using common EIP712 tools (e.g. using the signTypedData function from ethers.js) would not pass validation .

Impact

As hinted under Vulnerability Details, signatures generated using common EIP712 tools (e.g. using the signTypedData function from ethers.js) would not pass validation, which showcases how the current logic is broken.

Tools Used

Manual review

Recommendations

Re-encode the contents before computing the digest.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

finding-wrong-EIP712-encodeData

Invalid, `bytes32` is not a dynamic value as oppose to `bytes`, since it is exactly 32 bytes long, so the encoding is correct, no issues here that is inconsistent with the `encodeData` definition highlighted in EIP712. > The atomic values are encoded as follows: Boolean false and true are encoded as uint256 values 0 and 1 respectively. Addresses are encoded as uint160. Integer values are sign-extended to 256-bit and encoded in big endian order. bytes1 to bytes31 are arrays with a beginning (index 0) and an end (index length - 1), they are zero-padded at the end to bytes32 and encoded in beginning to end order. This corresponds to their encoding in ABI v1 and v2.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.