003.md

Howling Amethyst Moth

Medium

Rewards are Wrongly sent to msg.sender and not Claimer address set by the Staker/deposit owner

Summary

In an edge case when a user's address doesn't support the REWARD token, a user can simply user another address to claim this rewards. But the claim function sends fund to the msg.sender instead of forwarding to the claimer address specified.

Vulnerability Detail

The claim function forwards reward to msg.sender instead of to the claimer address.

A user can specify the account he/she wants to receive his/her reward

1. Depositor == Claimer

  function stake(uint256 _amount, address _delegatee)
    external
    virtual
    returns (DepositIdentifier _depositId)
  {

@audit>>    _depositId = _stake(msg.sender, _amount, _delegatee, msg.sender);
 
 }

2. Depositor != Claimer

  function stake(uint256 _amount, address _delegatee, address _claimer)
    external
    virtual
    returns (DepositIdentifier _depositId)
  {

@audit>>     _depositId = _stake(msg.sender, _amount, _delegatee, _claimer);
 
 }

3. Claimer can be altered

function alterClaimer(DepositIdentifier _depositId, address _newClaimer) external virtual {
   Deposit storage deposit = deposits[_depositId];
   _revertIfNotDepositOwner(deposit, msg.sender);

@audit>>     _alterClaimer(deposit, _depositId, _newClaimer);
 }

The Owner is allowed to call claim and the claimer is also allowed but this should forward the reward token to the claimer regardless of who is calling.

  /// @notice Claim reward tokens earned by a given deposit. Message sender must be the claimer
  /// address of the deposit. Tokens are sent to the claimer address.
  /// @param _depositId Identifier of the deposit from which accrued rewards will be claimed.
  /// @return Amount of reward tokens claimed, after the fee has been assessed.
  function claimReward(DepositIdentifier _depositId) external virtual returns (uint256) {
    Deposit storage deposit = deposits[_depositId];

@audit>>       if (deposit.claimer != msg.sender && deposit.owner != msg.sender) {
      revert GovernanceStaker__Unauthorized("not claimer or owner", msg.sender);
    }
 
@audit>>      return _claimReward(_depositId, deposit, msg.sender);

  }

This issue also occur when claiming using signature

  /// @notice Claim reward tokens earned by a given deposit, using a signature to validate the
  /// caller's intent. The signer must be the claimer address of the deposit Tokens are sent to
  /// the claimer.
  /// @param _depositId The identifier for the deposit for which to claim rewards.
  /// @param _deadline The timestamp after which the signature should expire.
  /// @param _signature Signature of the claimer authorizing this reward claim.
  /// @return Amount of reward tokens claimed, after the fee has been assessed.
  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, nonces(deposit.claimer), _deadline))
    );
    bool _isValidClaimerClaim =
      SignatureChecker.isValidSignatureNow(deposit.claimer, _claimerHash, _signature);
    if (_isValidClaimerClaim) {
      _useNonce(deposit.claimer);
      return _claimReward(_depositId, deposit, deposit.claimer);
    }

    bytes32 _ownerHash = _hashTypedDataV4(
      keccak256(abi.encode(CLAIM_REWARD_TYPEHASH, _depositId, _useNonce(deposit.owner), _deadline))
    );
    bool _isValidOwnerClaim =
      SignatureChecker.isValidSignatureNow(deposit.owner, _ownerHash, _signature);
    if (!_isValidOwnerClaim) revert GovernanceStakerOnBehalf__InvalidSignature();
    
@audit>>>      return _claimReward(_depositId, deposit, deposit.owner);
  }

As implemented also in UNISTAKER , The beneficiary is always the collector, Depositor can call on behalf of but beneficiary is always the collector

https://github.com/uniswapfoundation/UniStaker/blob/887d7dc0c1db3f17227d13af4d8a791a66912d42/src/UniStaker.sol#L565-L593

  /// @notice Claim reward tokens the message sender has earned as a stake beneficiary. Tokens are
  /// sent to the message sender.
  /// @return Amount of reward tokens claimed.
  function claimReward() external returns (uint256) {
    return _claimReward(msg.sender);
  }

  /// @notice Claim earned reward tokens for a beneficiary, using a signature to validate the
  /// beneficiary's intent. Tokens are sent to the beneficiary.
  /// @param _beneficiary Address of the beneficiary who will receive the reward.
  /// @param _deadline The timestamp after which the signature should expire.
  /// @param _signature Signature of the beneficiary authorizing this reward claim.
  /// @return Amount of reward tokens claimed.
  function claimRewardOnBehalf(address _beneficiary, uint256 _deadline, bytes memory _signature)
    external
    returns (uint256)
  {
    _revertIfPastDeadline(_deadline);
    _revertIfSignatureIsNotValidNow(
      _beneficiary,
      _hashTypedDataV4(
        keccak256(
          abi.encode(CLAIM_REWARD_TYPEHASH, _beneficiary, _useNonce(_beneficiary), _deadline)
        )
      ),
      _signature
    );
    return _claimReward(_beneficiary);
  }

https://github.com/uniswapfoundation/UniStaker/blob/887d7dc0c1db3f17227d13af4d8a791a66912d42/src/UniStaker.sol#L760-L778

 /// @notice Internal convenience method which alters the beneficiary of an existing deposit.
  /// @dev This method must only be called after proper authorization has been completed.
  /// @dev See public alterBeneficiary methods for additional documentation.
  function _alterBeneficiary(
    Deposit storage deposit,
    DepositIdentifier _depositId,
    address _newBeneficiary
  ) internal {
    _revertIfAddressZero(_newBeneficiary);
    _checkpointGlobalReward();
    _checkpointReward(deposit.beneficiary);
    earningPower[deposit.beneficiary] -= deposit.balance;

    _checkpointReward(_newBeneficiary);
    emit BeneficiaryAltered(_depositId, deposit.beneficiary, _newBeneficiary);
    deposit.beneficiary = _newBeneficiary;
    earningPower[_newBeneficiary] += deposit.balance;
  }

Impact

Reward tokens are sent to msg.sender/owner because he called the claim function instead of the claimer address, thereby breaking the core functionality of the Claim Reward function. If msg.sender does not support this token (e.g. contract), these tokens will be stuck in the contract/msg.sender.

Code Snippet

https://github.com/sherlock-audit/2024-11-tally/blob/main/staker/src/GovernanceStaker.sol#L403-L413

Tool used

Manual Review

Recommendation

Always forward the reward to the claimer address set by the owner in respective of the caller.

  function claimReward(DepositIdentifier _depositId) external virtual returns (uint256) {
    Deposit storage deposit = deposits[_depositId];
    if (deposit.claimer != msg.sender && deposit.owner != msg.sender) {
      revert GovernanceStaker__Unauthorized("not claimer or owner", msg.sender);
    }

--      return _claimReward(_depositId, deposit, msg.sender);

++    return _claimReward(_depositId, deposit, deposit.claimer);
  }

    bytes32 _ownerHash = _hashTypedDataV4(
      keccak256(abi.encode(CLAIM_REWARD_TYPEHASH, _depositId, _useNonce(deposit.owner), _deadline))
    );
    bool _isValidOwnerClaim =
      SignatureChecker.isValidSignatureNow(deposit.owner, _ownerHash, _signature);
    if (!_isValidOwnerClaim) revert GovernanceStakerOnBehalf__InvalidSignature();
 
--   return _claimReward(_depositId, deposit, deposit.owner);
++   return _claimReward(_depositId, deposit, deposit.claimer);
  }