Refined Bone Bat - Owner-Centralized Withdrawals in withdrawToken

[sherlock-admin2]

Refined Bone Bat

High

Owner-Centralized Withdrawals in withdrawToken

Summary

The unrestricted withdrawal capability in NumaVault will cause a complete loss of funds for all users as a malicious or compromised owner will call withdrawToken to transfer all assets from the vault.

Root Cause

In NumaVault.sol:1305, the withdrawToken function allows the owner to transfer any ERC20 token from the vault to any address without restrictions. This function lacks safeguards, such as governance approval, multi-signature requirements, or withdrawal limits.

Relevant Code:
NumaVault.sol:1305:
https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/NumaProtocol/NumaVault.sol#L1305

Internal pre-conditions

1-The owner role is controlled by a single private key or account.
2-The withdrawToken function is callable without time-locks or additional approvals.

External pre-conditions

1-The owner account is compromised (e.g., via phishing or key leakage).
2-The protocol relies on centralized control for sensitive operations.

Attack Path

-An attacker gains access to the owner’s private key (e.g., through phishing or malware).
-The attacker calls withdrawToken repeatedly, transferring all ERC20 tokens from the vault to their own address:
vault.withdrawToken(address(lstToken), vaultBalance, attackerAddress);

Complete Vault Depletion:

• All user funds held in the vault are drained, leaving it with a zero balance.

Impact

Affected Party: The protocol and all users.
Loss: Complete loss of user funds stored in the vault, totaling all ERC20 tokens under its custody.

• Gain: The attacker acquires all vault assets.

PoC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import "forge-std/Test.sol";
import "../contracts/NumaVault.sol";
import "../contracts/mocks/MockToken.sol";

contract NumaVaultWithdrawTest is Test {
    NumaVault vault;
    MockToken lstToken;
    address attacker = address(0x123);

    function setUp() public {
        // Deploy mock dependencies
        lstToken = new MockToken("LST Token", "LST", 18);
        vault = new NumaVault(
            address(lstToken),
            address(lstToken),
            18,
            address(0x0), // No oracle for this test
            address(this),
            0,
            0
        );

        // Fund the vault
        lstToken.mint(address(vault), 1_000e18);
    }

    function testOwnerWithdrawalExploit() public {
        // Simulate attacker gaining control of owner account
        vm.startPrank(attacker);

        // Execute withdrawal
        uint256 vaultBalance = lstToken.balanceOf(address(vault));
        vault.withdrawToken(address(lstToken), vaultBalance, attacker);

        // Assert vault is drained
        uint256 attackerBalance = lstToken.balanceOf(attacker);
        uint256 remainingVaultBalance = lstToken.balanceOf(address(vault));

        console.log("Attacker Balance:", attackerBalance);
        console.log("Vault Balance:", remainingVaultBalance);

        assertEq(attackerBalance, 1_000e18);
        assertEq(remainingVaultBalance, 0);
    }
}

Vault holds 1,000 LST tokens; attacker balance is 0.

• After Exploit: The attacker drains all 1,000 LST tokens from the vault using withdrawToken. Vault balance is 0.

Mitigation

1-Governance-Based Withdrawals:

• Replace onlyOwner with a governance-based approval mechanism for withdrawal operations:

modifier onlyGovernance {
    require(msg.sender == governance, "Not authorized");
    _;
}

2-Multi-Signature Requirements:

• Require multi-signature approval for critical operations like withdrawals to prevent single-point compromise.

3-Withdrawal Time-Lock:

• Introduce a time-lock for withdrawals, providing users with a window to respond to potential threats:

mapping(address => uint256) public withdrawalRequests;
uint256 public constant WITHDRAWAL_DELAY = 48 hours;


function requestWithdrawal(address _token, uint256 _amount) external onlyOwner {
    withdrawalRequests[_token] = block.timestamp + WITHDRAWAL_DELAY;
}

function executeWithdrawal(address _token, uint256 _amount, address _receiver) external onlyOwner {
    require(block.timestamp >= withdrawalRequests[_token], "Withdrawal delay not met");
    SafeERC20.safeTransfer(IERC20(_token), _receiver, _amount);
}

4-Withdraw Limits:
Implement limits on the amount that can be withdrawn in a single transaction.