code-423n4 / 2023-07-amphora-findings

3 stars 2 forks source link

Nonces are not used in the signature checks #408

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

in the contract GovernorCharlie.sol in function executeTransaction Nonces are not used in the signature

A nonce can prevent an old value from being used when a new value exists. Without one, two transactions submitted in one order, can appear in a block in a different order

Proof of Concept

here is instance: https://github.com/code-423n4/2023-07-amphora/blob/179384321c36b669f48bc0485bbc1f807fba8fac/core/solidity/contracts/governance/GovernorCharlie.sol#L337

 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/ vs code

Recommended Mitigation Steps

Include a nonce in what is signed

+    bytes32 _txHash = keccak256(abi.encode(_target, _value, _signature, _data, _eta, nonce));

instead of

 bytes32 _txHash = keccak256(abi.encode(_target, _value, _signature, _data, _eta));

Assessed type

Governance

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

Invalid

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Insufficient quality