Incorrect validation in `executeTransaction()` could lead to theft of ETH

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/governance/GovernorCharlie.sol#L322-L354

Vulnerability details

Impact

Unauthorized extraction of ETH from the contract.

Proof of Concept

The vulnerability lies within the executeTransaction function, since it accepts a _value parameter representing the Ethereum value of the transaction. This _value is later used in a low-level call function to send Ethereum to a _target address.

The function, however, does not verify that the provided _value matches the msg.value sent with the transaction. This means a caller can send a lower amount of Ethereum with the transaction than what they specify in the _value parameter, but the contract would still execute the transaction sending the larger _value of Ethereum to the _target address. Take a look at GovernorCharlie.sol#L322-L354

  /// @notice Executes a transaction
  /// @param _target Target address for transaction
  /// @param _value Eth value for transaction
  /// @param _signature Function signature for transaction
  /// @param _data Calldata for transaction
  /// @param _eta Timestamp for transaction
  function executeTransaction(
    address _target,
    uint256 _value,
    string memory _signature,
    bytes memory _data,
    uint256 _eta
  ) external payable override {
    if (msg.sender != address(this)) revert GovernorCharlie_NotGovernorCharlie();

    bytes32 _txHash = keccak256(abi.encode(_target, _value, _signature, _data, _eta));
    if (!queuedTransactions[_txHash]) revert GovernorCharlie_ProposalNotQueued();
    if (_getBlockTimestamp() < _eta) revert GovernorCharlie_TimelockNotReached();
    if (_getBlockTimestamp() > _eta + GRACE_PERIOD) revert GovernorCharlie_TransactionStale();

    queuedTransactions[_txHash] = false;

    bytes memory _callData;

    if (bytes(_signature).length == 0) _callData = _data;
    else _callData = abi.encodePacked(bytes4(keccak256(bytes(_signature))), _data);

    // solhint-disable-next-line avoid-low-level-calls
    (bool _success, /*bytes memory returnData*/ ) = _target.call{value: _value}(_callData);
    if (!_success) revert GovernorCharlie_TransactionReverted();

    emit ExecuteTransaction(_txHash, _target, _value, _signature, _data, _eta);
  }

With the relevant code in question being:

(bool _success, /*bytes memory returnData*/ ) = _target.call{value: _value}(_callData);
if (!_success) revert GovernorCharlie_TransactionReverted();

Tool used

Manual Audit

Recommended Mitigation Steps

Recommend that a check is introduced to ensure that msg.value equals _value or less than it. If not, the function should revert the transaction.

A proposed change to the code would be in the line of the below:

if (msg.value < _value) revert SomeRevertMessage();

Additional Notes

The contract should also implement a function that's somewhat like removeDus() to remove extra eth that could have been sent to contract, though the contract does not implement any payable fallback functions the execute() and executeTransaction() are marked payable so extra eth could land in contract through these functions, that creates an incentive for attackers to try stealing them as stated in report.

Assessed type

ETH-Transfer

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

executeTransaction() is only called from execute()

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid