Reentrancy Vulnerability in emergencyWithdraw Function with ERC-777 Tokens

Summary

The emergencyWithdraw function is vulnerable to reentrancy attacks when used with ERC-777 tokens or other tokens that have hooks that are called on transfer.

Vulnerability Detail

While the function uses the nonReentrant modifier, it's still vulnerable to reentrancy through ERC-777 tokens. These tokens call a hook on the recipient before updating the token balances, which can be exploited to reenter the emergencyWithdraw function. https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/Vault.sol#L401-L416

The vulnerability occurs because the function transfers tokens before updating the internal state:

Tokens are transferred to the user.
For ERC-777 tokens, this calls a hook on the receiving contract.
The attacker can use this hook to call emergencyWithdraw again.
The internal state hasn't been updated yet, so the second withdrawal is processed with the same initial conditions.

Impact

An attacker could potentially drain the vault of more tokens than they are entitled to, causing significant financial loss to the protocol.

Code Snippet

function emergencyWithdraw(
    uint256[] memory minAmounts,
    uint256 deadline
) external nonReentrant checkDeadline(deadline) returns (uint256[] memory actualAmounts) {
    // ... (code omitted for brevity)
    for (uint256 i = 0; i < tokens.length; i++) {
        // ... (code omitted for brevity)
        IERC20(tokens[i]).safeTransfer(request.to, amount);
        actualAmounts[i] = amount;
    }
    delete _withdrawalRequest[sender];
    _pendingWithdrawers.remove(sender);
    _burn(address(this), request.lpAmount);
    // ... (code omitted for brevity)
}

Here's a Foundry test that demonstrates the vulnerability:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Vault.sol";
import "../src/MockERC777.sol";

contract VaultTest is Test {
    Vault vault;
    MockERC777 token;
    address attacker = address(0x1);

    function setUp() public {
        vault = new Vault("Test Vault", "TVLT", address(this));
        token = new MockERC777("Mock Token", "MTK", new address[](0));

        // Setup vault with token
        vault.addToken(address(token));

        // Fund vault with tokens
        token.mint(address(vault), 1000 ether);

        // Setup attacker with some LP tokens and a withdrawal request
        vault.deposit(attacker, [100 ether], 100 ether, block.timestamp + 1 hours);
        vm.prank(attacker);
        vault.registerWithdrawal(attacker, 100 ether, [90 ether], block.timestamp + 1 hours, block.timestamp + 2 hours, false);
    }

    function testReentrancyAttack() public {
        vm.prank(attacker);
        vault.emergencyWithdraw([90 ether], block.timestamp + 1 hours);

        // Check that attacker received more tokens than they should have
        assertGt(token.balanceOf(attacker), 100 ether);
    }
}

contract MockERC777 is ERC777 {
    constructor(string memory name, string memory symbol, address[] memory defaultOperators) 
        ERC777(name, symbol, defaultOperators) {}

    function mint(address account, uint256 amount) public {
        _mint(account, amount, "", "", true);
    }
}

Tool used

Manual Review, Foundry

Recommendation

Implement a "checks-effects-interactions" pattern. Update the internal state before making external calls:

function emergencyWithdraw(
    uint256[] memory minAmounts,
    uint256 deadline
) external nonReentrant checkDeadline(deadline) returns (uint256[] memory actualAmounts) {
    // ... (code omitted for brevity)
    uint256 lpAmountToBurn = request.lpAmount;
    delete _withdrawalRequest[sender];
    _pendingWithdrawers.remove(sender);
    _burn(address(this), lpAmountToBurn);

    for (uint256 i = 0; i < tokens.length; i++) {
        // ... (code omitted for brevity)
        IERC20(tokens[i]).safeTransfer(request.to, amount);
        actualAmounts[i] = amount;
    }
    // ... (code omitted for brevity)
}

sherlock-audit / 2024-06-mellow-judging

mike-watson - Reentrancy Vulnerability in emergencyWithdraw Function with ERC-777 Tokens #108