GovernanceStakerOnBehalf nonces can be consumed for any address by any depositor

[CergyK]

Description

GovernanceStakerOnBehalf enables to carry actions on deposits on behalf of another user. These actions can be carried by using off-chain signatures, and nonces are implemented in order to avoid signature replay, which is a standard protection.

However for the endpoint claimRewardOnBehalf, it is not clear if the signature provided is one of the claimer of the deposit or owner of the deposit (both are accepted).

The signature for the claimer of the deposit is tested first, but it uses the nonce even if the signature is invalid for the claimer:
GovernanceStakerOnBehalf.sol#L259:

  function claimRewardOnBehalf(
    DepositIdentifier _depositId,
    uint256 _deadline,
    bytes memory _signature
  ) external virtual returns (uint256) {
    _revertIfPastDeadline(_deadline);
    Deposit storage deposit = deposits[_depositId];
    bytes32 _claimerHash = _hashTypedDataV4(
      keccak256(
>>      abi.encode(CLAIM_REWARD_TYPEHASH, _depositId, _useNonce(deposit.claimer), _deadline)
      )
    );

    ...
  }

Nonces.sol#L28-L35:

    function _useNonce(address owner) internal virtual returns (uint256) {
        // For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
        // decremented or reset. This guarantees that the nonce never overflows.
        unchecked {
            // It is important to do x++ and not ++x here.
            return _nonces[owner]++;
        }
    }

Scenario

A depositor Bob can bump any nonce for any address Alice by:

In one transaction:
1/ Setting Alice as a claimer for a deposit Bob owns
2/ Call claimRewardsOnBehalfOf with Bob signature
3/ Reset claimer to Bob controlled address.

Impact

Any action done on GovernanceStakerOnBehalf can be DOSed by a malicious actor bumping the nonce on behalf of the victim user

Recommendation

Only use the nonce when the signature has been confirmed as valid. Here instead of _useNonce, the function nonces() can be used to get the nonce:

GovernanceStakerOnBehalf.sol#L259:

  function claimRewardOnBehalf(
    DepositIdentifier _depositId,
    uint256 _deadline,
    bytes memory _signature
  ) external virtual returns (uint256) {
    _revertIfPastDeadline(_deadline);
    Deposit storage deposit = deposits[_depositId];
    bytes32 _claimerHash = _hashTypedDataV4(
      keccak256(
-      abi.encode(CLAIM_REWARD_TYPEHASH, _depositId, _useNonce(deposit.claimer), _deadline)
+      abi.encode(CLAIM_REWARD_TYPEHASH, _depositId, nonces(deposit.claimer), _deadline)
      )
    );

    ...
  }

And then the nonce should be used once the signature has been validated

[alexkeating]

We believe the severity of this issue should be lowered to low/info because any action can be triggered without an on behalf call. Also, a defender can somewhat mitigate this issue. An attacker must have claimable rewards greater than the fee amount, and in order to attack an address the attacker must allow the defender to claim their fees. In order to stop the ddos the defender could claim the attackers fees thus preventing the attacker from successfully calling claimRewardOnBehalf.

[CergyK]

True, given the preconditions and the unfrequent opportunity to trigger the bug (due to the claiming fee), we can safely downgrade this to low

[alexkeating]

Will fix

[CergyK]

Fixed by withtally/staker#89