Arbitrary User won't be able to stake on Behalf of Staker despite correct User Signature Intent

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L660

Vulnerability details

Proof of Concept

The UniStaker contract facilitates staking in multiple ways:

1. stake: Directly used by the staker to stake UNI tokens into the UniStaker Contract.

2. permitAndStake: Enables the staker to sign an ECDSA signature permitting the contract to transfer and stake tokens.

3. stakeOnBehalf: Intended for staking UNI tokens on behalf of another user, utilizing a provided user intent signature.

However, look at Natspec of stakeOnBehalf function which reveals an important point:

@->   /// @notice Stake tokens to a new deposit on behalf of a user, using a signature to validate the
      /// user's intent. The caller must pre-approve the staking contract to spend at least the
      /// would-be staked amount of the token.


@->   /// @param _depositor Address of the user on whose behalf this stake is being made.


      function stakeOnBehalf(

Here, it's clearly written that caller must pre-approve the staking contract to spend at least the would-be staked amount of the token..

Consider the following scenario:

1. Alice intends to stake X UNI tokens on behalf of Bob.
2. Bob signs the staking transaction and sends the signature to Alice.
3. Alice, as the caller, proceeds to invoke stakeOnBehalf, ensuring pre-approval of X tokens to the contract.
4. During execution, upon reaching the _stake function, funds transfer occurs from Bob to the designated _surrogate address, rather than from Alice to _surrogate, resulting in failure to stake funds on behalf of Bob because of following line.

660:    _stakeTokenSafeTransferFrom(_depositor, address(_surrogate), _amount);

Link to Code

This way, Alice fails to stake funds on behalf of Bob.

Impact

Inability to allow others to stake funds on behalf of user, despite possessing the correct signature (user intent).

Tools Used

VS Code

Recommended Mitigation Steps

Update the _stake function as follows to permit any entity to stake on behalf of a user, provided they possess the user's intent:

     function _stake(address _depositor, uint256 _amount, address _delegatee, address _beneficiary)
    internal
    returns (DepositIdentifier _depositId)
  {
      // SNIP //

-     _stakeTokenSafeTransferFrom(_depositor, address(_surrogate), _amount);
+     _stakeTokenSafeTransferFrom(msg.sender, address(_surrogate), _amount);

Assessed type

Context

[MarioPoneder]

Working as intended. Someone else pays for transaction stakeOnBehalf while funds are still transferred from _depositor.
However, the doc/comment is misleading, therefore valid QA.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b