Enum is not present in EIP-712

[hats-bug-reporter[bot]]

Github username: @SB-Security Twitter username: SBSecurity_ Submission hash (on-chain): 0xded09c8daa3665d393f5976c8f7e0bb2bf0720c579734fa1bb0f153a775f6a50 Severity: medium

Description: Description\ Enum.Operation is not common type and cannot be used in EIP-712 hash.

Attack Scenario\ Enums are derived from uint and uint should be used instead, using Enum.Operation will make the hash wrong. Attachments

1. Proof of Concept (PoC) File

   function createDigestExecTx(
       bytes32 domainSeparatorSafe,
       Transaction memory safeTx
   ) public view returns (bytes32) {
       bytes32 digest = _hashTypedDataV4(
           domainSeparatorSafe,
           keccak256(
               abi.encode(
                   keccak256(
                       "execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"
                   ),
                   safeTx.to,
                   safeTx.value,
                   safeTx.data,
                   safeTx.operation,
                   safeTx.safeTxGas,
                   safeTx.baseGas,
                   safeTx.gasPrice,
                   safeTx.gasToken,
                   safeTx.refundReceiver,
                   safeTx.signatures
               )
           )
       );
   
       return digest;
   }

2. Revised Code File (Optional)

[0xRizwan]

Doesn't seem to be an issue. Even safe has used enum in calculation of hashes.

[Slavchew]

No, they aren't. I'm speaking of the TYPEHASH, not the data.

The explanation is not the best, but the issue is that the TYPEHASH, should use uint8 instead of Enum.Operation.

"execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"

Can see the SAFE one here - https://github.com/safe-global/safe-smart-account/blob/499b17ad0191b575fcadc5cb5b8e3faeae5391ae/contracts/Safe.sol#L59-L62

[0xRizwan]

Using uint256 instead of uint8 for enum would produce incorrect hash. I think, this should be medium severity.

[alfredolopez80]

@0xRizwan Non-Issue like a explain in #6 becuase is not part of the core contract itself

[Slavchew]

What it means is not part of the main contract. The problem is in the scope and must be treated as valid.

As an auditor, I look at the scope, not whether the feature is used in the main contract or wherever.

[alfredolopez80]

What it means is not part of the main contract. The problem is in the scope and must be treated as valid.

As an auditor, I look at the scope, not whether the feature is used in the main contract or wherever.

like another contract is only for simulate/testing!! is the same for src/ReentrancyAttack.sol.sol is only for test it!!

[Slavchew]

What it means is not part of the main contract. The problem is in the scope and must be treated as valid. As an auditor, I look at the scope, not whether the feature is used in the main contract or wherever.

like another contract is only for simulate/testing!! is the same for src/ReentrancyAttack.sol.sol is only for test it!!

Then why are they in the scope, since will not be used?

@0xRizwan can you leave your point here? How are auditors supposed to guess which contract of those in the scope they should actually look at.

[Slavchew]

I think this one is a valid Medium. I can express the same thoughts as in https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/issues/1#issuecomment-2193871098

The issue has the same root case as this one - https://github.com/code-423n4/2023-10-brahma-findings/issues/23 which was previously marked as Medium