hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 2 forks source link

Constant Should be defined rather than Use of Magic Number in `convertEVMTo65` Function #9

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: @ihtisham-sudo Twitter username: ihtishamSudo Submission hash (on-chain): 0xcca53c9ec38358467664138aa3095be463152b9e98e4c9961f3f83999fe119e9 Severity: low

Description: Description\ The convertEVMTo65 function in the Bytes65.sol file uses a magic number (uint8(20)). And breaking changes in hard fork can lead to many issues that can totally break the magic or hardcoded numbers. Using magic numbers directly in code can make the code less readable, maintainable, and may lead to potential errors in the future.

Attack Scenario\ See PoC

Attachments https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/utils/Bytes65.sol#L18

  1. Proof of Concept (PoC) File

    function convertEVMTo65(address evmAddress) public pure returns(bytes memory) {
        return abi.encodePacked(
            uint8(20),                              // Size of address. Is always 20 for EVM
            bytes32(0),                             // First 32 bytes on EVM are 0
            bytes32(uint256(uint160((evmAddress)))) // Encode the address in bytes32.
        );
    }
  2. Revised Code File (Optional) Define a constant for the magic number 20 and use it in the convertEVMTo65 function to improve code readability and maintainability. Note: Replace the constant name (EVM_ADDRESS_SIZE) with a more meaningful name if necessary, based on the specific context in your code.

// Define a constant for the size of the address
+uint8 constant private EVM_ADDRESS_SIZE = 20;

function convertEVMTo65(address evmAddress) public pure returns(bytes memory) {
    return abi.encodePacked(
-        uint8(20),
+        EVM_ADDRESS_SIZE,                        // Use the constant instead of magic number
        bytes32(0),                              // First 32 bytes on EVM are 0
        bytes32(uint256(uint160((evmAddress)))) // Encode the address in bytes32.
    );
}

Files:

reednaa commented 6 months ago

It would still break because of the intermediate conversion into uint160. And length uint8(20) is never used on EVM (except in other place I think), the implementation specifics are for other VMs where address sizes matter.