UniStaker.sol#permitAndStake() and UniStaker.sol#permitAndStakeMore() functions are allowed to be called by permit creator only (incorrect implementation of EIP-2612)

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

UniStaker.sol#permitAndStake() and UniStaker.sol#permitAndStakeMore() functions are allowed to be called by permit creator only. No any other contracts will be able to execute these function on behalf of depositor (initial depositor in permit and stake situation). (incorrect implementation of EIP-2612)

• permitAndStake() function

  function permitAndStake(
    uint256 _amount,
    address _delegatee,
    address _beneficiary,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external returns (DepositIdentifier _depositId) {
    STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); // @audit can be called by permit creator only (msg.sender)
    _depositId = _stake(msg.sender, _amount, _delegatee, _beneficiary);
  }

• permitAndStakeMore() function

  function permitAndStakeMore(
    DepositIdentifier _depositId,
    uint256 _amount,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external {
    Deposit storage deposit = deposits[_depositId];
    _revertIfNotDepositOwner(deposit, msg.sender);

    STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); // @audit can be called by permit creator only (msg.sender)
    _stakeMore(deposit, _depositId, _amount);
  }

Proof of Concept

The UniStaker.sol contract in the UniStaker Infrastructure is designed to manage the staking of governance tokens (including and delegation) and distribution of rewards (including and beneficiaries). Among its features, the UniStaker.sol contract implements permitAndStake() and permitAndStakeMore() functions, which leverage the ERC-2612 standard to allow token holders to give permission to the contract to stake tokens on their behalf using a signed message, instead of requiring a separate transaction for approval. This mechanism aims to improve interactions by reducing the number of transactions and the associated gas costs.

• permitAndStake(): Allows a user to stake tokens by providing a signed message, granting permission to the contract to move tokens on their behalf.
• permitAndStakeMore(): Enables a user to add more tokens to an existing stake using a similar permissioning approach.

The implementation of these functions aims to improve user experience within the staking process by integrating EIP-2612's permit standard/feature, which allows for gas-less approvals through signed messages.

But, what is the problem?
The problem is that the current implementation restricts the permitAndStake() and permitAndStakeMore() functions to be called by the permit creator only, which differs from the intended flexibility of EIP-2612 Standard.
The purpose of permit is to allow someone to sign approve signature, so that this signature can be used by another contract to call some function on behalf of signer. In this case, anyone should be able to sign permit for the staking and staking more actions.
Also permitAndStakeMore() functions in UniStaker.sol contract check if the _depositor is same who signed permit.

```solidity
  _revertIfNotDepositOwner(deposit, msg.sender);
```

As a result, users cannot delegate staking actions (staking and staking more) to a third party or smart contract, which can make possible batch transactions or integrations with other DeFi protocols.

• permitAndStake() function function:

  function permitAndStake(
    uint256 _amount,
    address _delegatee,
    address _beneficiary,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external returns (DepositIdentifier _depositId) {
    STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); //@audit the function incorrectly enforce that the caller (msg.sender) of the function must be the same entity that created the permit, contradicting the `EIP-2612` intention that permits allow for more flexible interactions through signed messages.
    _depositId = _stake(msg.sender, _amount, _delegatee, _beneficiary);
  }

• permitAndStakeMore() function function:

  function permitAndStakeMore(
    DepositIdentifier _depositId,
    uint256 _amount,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external {
    Deposit storage deposit = deposits[_depositId];
    _revertIfNotDepositOwner(deposit, msg.sender);

    STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); //@audit the function incorrectly enforce that the caller (msg.sender) of the function must be the same entity that created the permit, contradicting the `EIP-2612` intention that permits allow for more flexible interactions through signed messages.
    _stakeMore(deposit, _depositId, _amount);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

To align with the intended use of EIP-2612, I recommend the following mitigation: Adjust the permitAndStake() and permitAndStakeMore() functions to remove the requirement that the msg.sender must be the permit creator. Instead, validate the permit against the intended staker's address provided in the signed message (signer of the permit, not the msg.sender).

Assessed type

Context

[c4-judge]

MarioPoneder marked the issue as duplicate of #4

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Invalid

[radeveth]

Hi, @MarioPoneder. Thank you for the quick judging!
I strongly believe the issue has been misjudged and should be mitigated to Medium Severity Issue. Let me explain.

Before I start with my escalation, I think the selected `primary issue" (#4) of this bug doesn't explain the whole bug well.

First, it's essential to clarify the nature of EIP-2612, the purpose of UniStaker.sol#permitAndStake() and UniStaker.sol#permitAndStakeMore() functions and the expectations for general user interaction with DeFi protocols that leverage EIP-2612:

1. Instead of utilizing an on-chain transaction, EIP-2612 suggests a standard that would enable token holders to grant spending authorization to a different address via a signed message. By lowering transaction costs and streamlining contract exchanges, this standard aims to improve user experience. The permit function is introduced, allowing for this off-chain signed consent.
2. While it's correct that permitAndStake(...) and permitAndStakeMore(...) are not defined within EIP-2612 itself, these functions are designed to extend the utility of EIP-2612's permit functionality within the UniStaker Protocol. They allow users to stake tokens or add to their stake directly after granting permission through a signed message, aligning with EIP-2612's goal of streamlined and efficient contract interactions.

As noted in my report:

The UniStaker.sol contract in the UniStaker Infrastructure is designed to manage the staking of governance tokens (including and delegation) and distribution of rewards (including and beneficiaries). Among its features, the UniStaker.sol contract implements permitAndStake() and permitAndStakeMore() functions, which leverage the ERC-2612 standard to allow token holders to give permission to the contract to stake tokens on their behalf using a signed message, instead of requiring a separate transaction for approval. This mechanism aims to improve interactions by reducing the number of transactions and the associated gas costs.
…
But, what is the problem?
The problem is that the current implementation restricts the permitAndStake() and permitAndStakeMore() functions to be called by the permit creator only, which differs from the intended flexibility of EIP-2612 Standard.
The purpose of permit is to allow someone to sign approve signature, so that this signature can be used by another contract to call some function on behalf of signer. In this case, anyone should be able to sign permit for the staking and staking more actions.

As a result, users cannot delegate staking actions (staking and staking more) to a third party or smart contract, which can make possible batch transactions or integrations with other DeFi protocols.

Any validation needs to handled in STAKE_TOKEN.permit(...) which is an external risk that should be discussed in the Analysis report (from judge comment)

My response: While the validation of the permit is indeed handled within the STAKE_TOKEN.permit(...) call, the issue at hand is not about the validation process itself but about the restrictive design of permitAndStake(...) and permitAndStakeMore(...) functions. These functions currently enforce that only the permit creator (i.e., msg.sender) can call them, which unnecessarily limits the utility of the signed permit. EIP-2612's design allows for permits to enable actions on the signer's behalf by any party, not just the signer themselves. This flexibility is very important for enabling a wide range of DeFi interactions, such as staking on behalf of another address, batch transactions, and integration with other contracts or protocols.

This comment from @LyuboslavVeliev also outlines the whole idea of the issue very well.

So, I think that the bug is valid because it restricts the potential of EIP-2612's permit functionality within the UniStaker. The current implementation limits these functions to be called only by the permit creator, contrary to the intended flexibility of EIP-2612, which aims to enhance user experience and interaction efficiency with smart contracts.

I respectfully request a reassessment of the issue, considering its significant impact, as I feel its severity has been underestimated and believe it should be reclassified as a Medium Severity Issue.

Have a good one! 🙂

[MarioPoneder]

Hi and thank you for your comment!

First of all, the discussed methods are not part of the ERC-2612 spec and therefore do not need to comply. Furtmore, those methods are working as intended, see also this comment. The on-behalf methods, which exhibit the behavior your are looking for, are already implemented in the codebase.

Therefore, leaving this as it is.

[radeveth]

Hi @MarioPoneder! Please see my report and my comment again because I think you are misunderstanding my point and the whole issue.

First of all, the discussed methods are not part of the ERC-2612 spec and therefore do not need to comply.

Incorrect, the all permit actions should comply with ERC-2612.

Furtmore, those methods are working as intended, see also this #4 (comment).

Incorrect, work as the developers intended, but it's a problem and needs to be fixed.

The on-behalf methods, which exhibit the behavior your are looking for, are already implemented in the codebase.

Incorrect, the on-behalf functions doesn't exhibit the behavior for that I am looking. The purpose of permit is to allow someone to sign off-chain approve signature, so that this signature can be used by another contract to call some function on behalf of signer. In this case, anyone should be able to sign permit for the staking and staking more actions. The correct implementation of permit functions aims to improve user experience within the staking process by integrating EIP-2612's permit standard/feature, which allows for gas-less approvals through signed messages.
So ultimately, if depositors don't have the gas to approve someone on-chain via on-behalf functions, then they should be able to do so off-chain.

I'll also note here that you chose #205 as a valid medium severity issue because if depositors want to invalidate their signatures, they have to do 1 extra transaction.

[radeveth]

@apbendi, @MarioPoneder

Could you review this report and my comments again. I will really appreciate it!