executeTransaction function is susceptible to signature malleability which allows replay attacks

[code423n4]

Lines of code

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

Vulnerability details

Impact

there is function that is susceptible to signature malleability which allows replay attacks in the contract GovernorCharlie.sol especially in executeTransaction( which could be very dangerous and lead to unwanted results like replay attacks in executing transactions.

A signature should never be included into a signed message hash to check if previous messages have been processed by the contract.

Proof of Concept

See reference: https://swcregistry.io/docs/SWC-117

instances : https://github.com/code-423n4/2023-07-amphora/blob/179384321c36b669f48bc0485bbc1f807fba8fac/core/solidity/contracts/governance/GovernorCharlie.sol#L328-L354

as you see signature is included in the signed message hash below.

 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);
  }

Tools Used

Manually / VsCode

Recommended Mitigation Steps

• consider not including the signature in the signed message hash.

Assessed type

Governance

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Invalid

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid

[dmvt]

Also insufficient proof, overinflated severity, and insufficient quality