`relay` Function: Reverts If It Intends to Send Native Tokens

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol#L103-L110 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L203-L210 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L254-L261

Vulnerability details

Impact

• realy function which exists in the security council management contracts enables the owners of these contracts to make calls to other contracts.

• But if native tokens is intended to be sent with the call; it will revert as the function is not payable,and there's noway for these management contracts to receive native tokens since receive() function is not implemented in these contracts.

Proof of Concept

• Code:

  SecurityCouncilMemberElectionGovernor contract/relay function

  File:governance/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol
  Line 103-110:
    function relay(address target, uint256 value, bytes calldata data)
        external
        virtual
        override
        onlyOwner
    {
        AddressUpgradeable.functionCallWithValue(target, data, value);
    }

  SecurityCouncilMemberRemovalGovernor contract/relay function

  File: governance/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol
  Line 203-210:
    function relay(address target, uint256 value, bytes calldata data)
        external
        virtual
        override
        onlyOwner
    {
        AddressUpgradeable.functionCallWithValue(target, data, value);
    }

  SecurityCouncilNomineeElectionGovernor contract/relay function

  File: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol
  Line 254-261:
    function relay(address target, uint256 value, bytes calldata data)
        external
        virtual
        override
        onlyOwner
    {
        AddressUpgradeable.functionCallWithValue(target, data, value);
    }

• Foundry PoC:

1. testRelayWithValue() test is added to SecurityCouncilMemberElectionGovernorTest.t.sol file; where the relay function is invoked by the contract owner to call a contract address: first without a value where it will be executed, then with a value where the call will revert due to lack of funds.

2. first a ReceiverContract contract is added to the governance/test/security-council-mgmt/governors/ directory where the calls from the relay function are going to target the receiveFunds() functionin in this contract:

   // SPDX-License-Identifier: Apache-2.0
   pragma solidity 0.8.16;
   
   contract ReceiverContract {
   function receiveFunds() public payable {}
   }

3. The testRelayWithValue() test: add this import at the top of the SecurityCouncilMemberElectionGovernorTest.t.sol test file:

   import "./ReceiverContract.sol";

   function testRelayWithValue() public {
       //0. deploy the receiver contract
       ReceiverContract receiverContract = new ReceiverContract();
       uint256 valueToSend = 10 ether;
   
       // 1. memberElection contract owner calls receiverContract::receiveFunds via relay without a value, it will be executed:
       assertEq(address(receiverContract).balance, 0);
       vm.startPrank(initParams.owner);
       governor.relay(
       address(receiverContract),
       0,
       abi.encodeWithSelector(receiverContract.receiveFunds.selector)
       );
       assertEq(address(receiverContract).balance, 0);
   
       // 2. memberElection contract owner calls receiverContract::receiveFunds via relay with a value, it will revert:
       vm.expectRevert();
       governor.relay(
       address(receiverContract),
       valueToSend,
       abi.encodeWithSelector(receiverContract.receiveFunds.selector)
       );
       assertEq(address(receiverContract).balance, 0);
   }

4. Test result:

   $ forge test --match-test testRelayWithValue
   [PASS] testRelayWithValue() (gas: 75277)
   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.40ms
   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Update relay function to be payable:

    function relay(address target, uint256 value, bytes calldata data)
        external
        virtual
        override
        onlyOwner
+       payable
    {
+       require(msg.value==value,"insufficient msg.value");
        AddressUpgradeable.functionCallWithValue(target, data, value);
    }

Assessed type

ETH-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #135

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[c4-judge]

0xean changed the severity to QA (Quality Assurance)