Operators can unfairly grief stakers deposits and delay undelegation

[code423n4]

Lines of code

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

Vulnerability details

Impact

The StrategyManager.depositIntoStrategyWithSignature() function unfairly provides malicious operators the ability to delay undelegation against stakers

Proof of Concept

StrategyManager.sol#L253

function depositIntoStrategyWithSignature(
    IStrategy strategy,
    IERC20 token,
    uint256 amount,
    address staker,
    uint256 expiry,
    bytes memory signature
)
    external
    onlyWhenNotPaused(PAUSED_DEPOSITS)
    onlyNotFrozen(staker)
    nonReentrant
    returns (uint256 shares)
{
    require(
        expiry >= block.timestamp,
        "StrategyManager.depositIntoStrategyWithSignature: signature expired"
    );
    // calculate struct hash, then increment `staker`'s nonce
    uint256 nonce = nonces[staker];
    bytes32 structHash = keccak256(abi.encode(DEPOSIT_TYPEHASH, strategy, token, amount, nonce, expiry));
    unchecked {
        nonces[staker] = nonce + 1;
    }

    bytes32 digestHash;
    //if chainid has changed, we must re-compute the domain separator
    if (block.chainid != ORIGINAL_CHAIN_ID) {
        bytes32 domain_separator = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes("EigenLayer")), block.chainid, address(this)));
        digestHash = keccak256(abi.encodePacked("\x19\x01", domain_separator, structHash));
    } else {
        digestHash = keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, structHash));
    }

    /**
        * check validity of signature:
        * 1) if `staker` is an EOA, then `signature` must be a valid ECSDA signature from `staker`,
        * indicating their intention for this action
        * 2) if `staker` is a contract, then `signature` must will be checked according to EIP-1271
        */
    if (Address.isContract(staker)) {
        require(IERC1271(staker).isValidSignature(digestHash, signature) == ERC1271_MAGICVALUE,
            "StrategyManager.depositIntoStrategyWithSignature: ERC1271 signature verification failed");
    } else {
        require(ECDSA.recover(digestHash, signature) == staker,
            "StrategyManager.depositIntoStrategyWithSignature: signature not from staker");
    }

    shares = _depositIntoStrategy(staker, strategy, token, amount);
}

Stakers can delegate to operators to deposit on behalf of staker for ERC20 tokens (currently only LST) by providing operators with required signatures. Although stakers are expected to do due diligence before delegating operations to operators, the current implementation of StrategyManager.depositIntoStrategyWithSignature allow operators to manipulate the expiry of signatures and deposit on behalf of stakers regardless of whether the signature has expired or not. Stakers will have to call queueWithdrawal before undelegation, which can be troublesome since protocol enforces delay in withdrawals.

Since there is no delays on deposits, this gives malicious operators the ability to delay undelegation say in the event of them not fulfilling its obligations in the services it participates in, potentially even causing stakers assets to be unecessarily slashed.

require(
    expiry >= block.timestamp,
    "StrategyManager.depositIntoStrategyWithSignature: signature expired"
);

Tools Used

Manual Analysis

Recommendation

Remove expiry input data and checks

Assessed type

Invalid Validation

[0xSorryNotSorry]

The submission does not provide well demonstration of the issue and reasoning especially on how operators can manipulate the expiry of signatures and deposit on behalf of stakers regardless of whether the signature has expired or not part. In addition, removing the expiry >= block.timestamp requirement creates an open check to the operator.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[GalloDaSballo]

The operator could not broadcast the message, but the same logic (irrational), could be applied to the user User self denying the functionality cannot be considered a valid finding (above QA)

For this reason am closing as overly inflated

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity