search

hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 1 forks source link

DomainSeparator is missing name and version #33

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: -- Twitter username: SBSecurity_ Submission hash (on-chain): 0xc5777f38ed6f17f0a268b41afcc5a3c2074c9c8f6a8c02e9cbecdf4ef53da006 Severity: medium

Description: Description\ DomainSeparator is missing name and version

Attack Scenario\ As per EIP-712 DomainSeparator should have name, version, chainId and verifyingContract, but the Palmera implemenattion is missing name and version

 /// @dev keccak256(
    ///     "EIP712Domain(uint256 chainId,address verifyingContract)"
    /// );
    bytes32 internal constant DOMAIN_SEPARATOR_TYPEHASH =
        0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;

It should add name and version like in OZ implementation - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/c3f8b760ad9d0431f33e8303658ccbdcc1610f24/contracts/utils/cryptography/EIP712.sol#L88-L90

Attachments

  1. Proof of Concept (PoC) File

function domainSeparator() public view returns (bytes32) { return keccak256( abi.encode(Constants.DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this) ); }



2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->
0xRizwan commented 1 week ago

This seems to be intended by Palmera. Please note, name and version is optional and upto protocol developers to include it as a part of domainSeparator hash.

where the type of eip712Domain is a struct named EIP712Domain with one or more of the below fields. Protocol designers only need to include the fields that make sense for their signing domain

Update: name and version is not a part of domainSeparator hash and DOMAIN_SEPARATOR_TYPEHASH only contains chainID and verifying contract i.e address(this) so this is intended design, if it was opposite then it could be considered a issue.

alfredolopez80 commented 1 week ago

First of all, thank you for your observation, below is our feedback on the matter.

The method domainSeparator already exists in the contract, you can validate it in the abstract contract Helpers.sol on this line: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/Helpers.sol#L44

Additional like mention @0xRizwan It is not our intention to include the name and version in the domain separator.