Missing `msg.sender` field in signature for deposit action

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L268

Vulnerability details

Impact

According to documentation, the depositIntoStrategyWithSignature method allows a user (authorized by a staker) to stake it. This is done for:

     * @dev A signature is required for this function to eliminate the possibility of griefing attacks, specifically those
     * targetting stakers who may be attempting to undelegate.

But the msg.sender field is missing in the signature, which allows anyone to use the signature and send a transaction on their own behalf (and not from the address where this signature was provided)

Theoretical impact:

• DoS of depositIntoStrategyWithSignature
• Violation of the requirements for which it was done possibility of griefing attacks
• This method is intended to make intermediate deposits, through the layer contract and to prevent various problems, but in fact we can see that we can use the signature for our needs, it can also cause future effects in the contract layers

Proof of Concept

We can see the absence of sender in the signature:

File: src\contracts\core\StrategyManagerStorage.sol

20:     bytes32 public constant DEPOSIT_TYPEHASH =
21:         keccak256("Deposit(address strategy,address token,uint256 amount,uint256 nonce,uint256 expiry)");

File: src\contracts\core\StrategyManager.sol

248:  function depositIntoStrategyWithSignature(
249:         IStrategy strategy,
250:         IERC20 token,
251:         uint256 amount,
252:         address staker,
253:         uint256 expiry,
254:         bytes memory signature
255:     )
256:         external
257:         onlyWhenNotPaused(PAUSED_DEPOSITS)
258:         onlyNotFrozen(staker)
259:         nonReentrant
260:         returns (uint256 shares)
261:     {

267:         uint256 nonce = nonces[staker];
-268:         bytes32 structHash = keccak256(abi.encode(DEPOSIT_TYPEHASH, strategy, token, amount, nonce, expiry)); //@audit
269:         unchecked {
270:             nonces[staker] = nonce + 1;
271:         }
272: 
273:         bytes32 digestHash;
274:         //if chainid has changed, we must re-compute the domain separator
275:         if (block.chainid != ORIGINAL_CHAIN_ID) {
-276:             bytes32 domain_separator = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes("EigenLayer")), block.chainid, address(this))); //@audit
277:             digestHash = keccak256(abi.encodePacked("\x19\x01", domain_separator, structHash));
278:         } else {
279:             digestHash = keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, structHash));
280:         }

-290:             require(IERC1271(staker).isValidSignature(digestHash, signature) == ERC1271_MAGICVALUE, //@audit
291:                 "StrategyManager.depositIntoStrategyWithSignature: ERC1271 signature verification failed");

-293:             require(ECDSA.recover(digestHash, signature) == staker, //@audit
294:                 "StrategyManager.depositIntoStrategyWithSignature: signature not from staker");

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Add msg.sender to signature, which will allow you to specify which contract layers can use this signature

Assessed type

Access Control

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[Sidu28]

This seems to be misinterpretting the intended usage of the function. The funds are transferred out of the msg.sender's wallet by design, not the specified staker's wallet.

[c4-sponsor]

Sidu28 marked the issue as sponsor disputed

[GalloDaSballo]

Siding with sponsor due to lack of proof around a broken invariant

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[BhHaipls]

Hi, @GalloDaSballo

I would like to clarify an example of a scenario that might not satisfy the staker.

When submitting the issue, I was guided by the docs/EigenLayer-deposit-flow.md documentation.

depositIntoStrategyWithSignature the new shares are credited to a specified staker, who must have also signed off on the deposit (this enables more complex, contract-mediated deposits, while a signature is required to mitigate the possibility of griefing or dusting-type attacks).

In the case when the signature is used directly by another user, there is nothing wrong with this, as they will simply transfer their funds to the staker.

However, the situation is different if there are contract-mediated deposits. Point 3 in Impact - This method ...

Imagine that in the system there are 2 different intermediary contracts (A, B) that provide different opportunities/obligations/conditions.

The user gives permission for them to stake through the logic of contract A. But in fact, anyone can stake through the logic of contract B. This leads to possible problems/loss of benefit.

Although this is all purely theoretical and depends on many other conditions for these contracts to actually be present in the future, not having additional checks, etc.

Perhaps I misunderstood what this enables more complex, contract-mediated deposits, while a signature is required to mitigate the possibility of griefing or dusting-type attacks means in the context of depositIntoStrategyWithSignature so I apologize for wasting your time.

Thank you, have a good day

[GalloDaSballo]

Siding with the sponsor on this one, the caller is "taking" the signature and giving all of the value for this

In lack of a rational reason to grief someone with such generosity, I must maintaining the finding as invalid

I do however, recommend you follow up with the Sponsor once an implementation of the Strategies is known, in case you can produce an exploit