Front-Running Vulnerability in `LRTDepositPool.depositAsset()`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L132-L134

Vulnerability details

Impact

The identified vulnerability in the LRTDepositPool.sol contract is a front-running issue in the depositAsset function. The nature of the vulnerability allows an attacker to observe a pending transaction intending to deposit assets close to the maximum limit and then execute a transaction with a higher gas price to be mined first. This could lead to the original transaction failing due to insufficient remaining deposit limit.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L132-L134

        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

In DeFi environments, front-running can be economically incentivized, especially if the attacker can benefit from blocking or influencing the outcome of other transactions. This issue can be particularly problematic in high-liquidity environments or during periods of significant network activity, where the timing of transactions can be critical for users.

Proof of Concept

Here's a typical scenario:

1. Bob's funding isn't currently available as getAssetCurrentLimit() has limited availability left.
2. He would constantly watch the mempool and frontrun anyone attempting to use up the remaining quota.
3. If Alice attempted to use up the last 10 ether of getAssetCurrentLimit(), Bob would frontrun her with 1 wei of depositAmount so that Alice's depositAmount of 10 ether ended up 1 wei greater than 10 ether - 1 of getAssetCurrentLimit().
4. If Alice attempted to use up the last 9.5 ether of getAssetCurrentLimit(), Bob would frontrun her with 0.5 ether + 1 of depositAmount just enough to make Alice's transaction revert.

Tools Used

Manual

Recommended Mitigation Steps

1. Minimum Deposit Threshold: Set a minimum deposit amount threshold. If the available limit (getAssetCurrentLimit(asset)) falls below this threshold, the transaction would revert unless certain conditions are met (like the sender having a non-zero balance of rsethToken). This threshold should be carefully chosen based on the typical transaction sizes and the economics of your system.
2. Checking rsethToken Balance: By checking if msg.sender has a non-zero balance of rsethToken, you allow users who are already participants in the system (and thus potentially have a vested interest) to make smaller deposits. This can be seen as a form of loyalty or trust system, where existing users are given more flexibility.
3. Implementation in the Contract: The logic in your depositAsset function would look something like this to use up the remaining getAssetCurrentLimit(asset) conditionally:

function depositAsset(address asset, uint256 depositAmount) external {
    uint256 availableLimit = getAssetCurrentLimit(asset);
    uint256 rsethBalance = IRSETH(rsethToken).balanceOf(msg.sender);

    if (depositAmount > availableLimit) {
        if (availableLimit < MIN_DEPOSIT_THRESHOLD && rsethBalance == 0) {
            revert DepositAmountTooLow();
        }
        depositAmount = availableLimit;
    }

    // ... rest of the function
}

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Inadequate use of quota limit.

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

gus-staderlabs (sponsor) disputed

[gus-stdr]

Though we see the value of this, we agree that this is not a KELPDAO protocol issue but an unintended consequence of the ETH blockchain design.

It is a nice to have, but not a bug in the system.

[c4-sponsor]

gus-staderlabs marked the issue as disagree with severity

[c4-sponsor]

manoj9april marked the issue as agree with severity

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[fatherGoose1]

I do not view this as a bug or anything to be fixed within Kelp's implementation. The likelihood that a griefer is willing to spend significant ETH gas fees to minorly inconvenience a depositor is low.