Slashing may sometimes fail for stETH vaults due to its 1-2 wei corner problem

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/SlashingHandler.sol#L56

Vulnerability details

Impact

Slashing stETH vaults may sometimes fail and always revert due to its innate 1-2 wei corner issue.

Proof of Concept

1. Context

The protocol plans on working with all ERC20 tokens, and that includes stETH. stETH however, has intrinsic rounding down problems. It's a known issue, and the stETH balance of an account could be lower of 1-2 wei because of rounding down. As such the amount received during transfer can be short 1 or 2 wei. This isn't much, but in context of slashing, in which an amount is transferred from the vault is also expected to be the amount transferred to address(0), this can mean unexpected failure of the handleSlashing function, potentially dossing/delaying slashing of malicious operators.

2. Bug Location

In SlashingHandler.sol, we see the handleSlashing function. The function transfers the amount of tokens to slash from the vault to the handler. After, the function attempts to transfer the "same" amount address(0).

    function handleSlashing(IERC20 token, uint256 amount) external {
        if (amount == 0) revert ZeroAmount();
        if (!_config().supportedAssets[token]) revert UnsupportedAsset();

        SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount);
        // Below is where custom logic for each asset lives
        SafeTransferLib.safeTransfer(address(token), address(0), amount);
    }

3. Final effect
   As explained that the amount received during transfer can be 1 or 2 wei shorter, the function's attempt to send the same amount fails and slashing is not completed.

4. Runnable POC

The gist link below contains modifications of some of the provided tests, including a mock token that simulates stETH losing 1 wei upon transfers, and instructions on how to run them.

https://gist.github.com/ZanyBonzy/033ee1ca8aae4969a676d8ce83a68c9d

The expected should look like this with an error that reads ERC20InsufficientBalance

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend querying token balances before and after every transfer, and transferring the difference between them instead.

Assessed type

Token-Transfer

[c4-sponsor]

@devdks25 Sponsors are not allowed to close, reopen, or assign issues or pull requests.

[devdks25]

Rebasing tokens aren't supported by the vaults. @dewpe

[dewpe]

This issue is taken care of by the transferAmount = Math.min(totalAssets(), totalAssetsToSlash); on L198 of the Vault.sol because both the balanceOf and transfer functions of the stETH vanilla contract call getPooledEthByShares underneath so they both would have the 1-2 gwei rounding issue. The Math.min would then account for the 1-2 gwei underflow

[MiloTruck]

Invalid.

Under the hood, stETH's transfer() functions calculates the amount of shares to transfer with getSharesByPooledEth():

function getSharesByPooledEth(uint256 _ethAmount) public view returns (uint256) {
    return _ethAmount
        .mul(_getTotalShares())
        .div(_getTotalPooledEther());
}

For the second transfer to revert, getSharesByPooledEth(amount) in the second transfer has to be less than getSharesByPooledEth(amount) in the first. However, as _ethAmount, _getTotalShares() and _getTotalPooledEther() are the same for both transfers, the amount of shares transferred will be the same. It is not possible for the second transfer to revert.

If the warden believes otherwise, consider providing a PoC that forks mainnet and actually interacts with stETH.

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid