dirk_y - A user that withdraws through a smart contract could lose funds forever in VUSD

[sherlock-admin]

dirk_y

medium

A user that withdraws through a smart contract could lose funds forever in VUSD

Summary

A user/business/protocol that holds vUSD and wants to redeem back to USDC could lose their funds forever if they try to redeem through a smart contract that has a bug or intermittent issue.

Vulnerability Detail

The VUSD contract allows a user to mint hUSD/vUSD by depositing the Hubble gas token (USDC). Similarly, a user can withdraw their deposited USDC by burning the hUSD funds they hold.

The withdrawal process takes place in 2 steps. First, the user has to call withdraw or withdrawTo. These methods burn the corresponding vUSD amount from the sender and add the withdrawal to a queue:

burn(amount); // burn vusd from msg.sender
withdrawals.push(Withdrawal(to, amount * SCALING_FACTOR));

The second step to finalising a withdrawal is to call processWithdrawals. This method loops through all of the withdrawals in the queue and calls:

            (bool success, bytes memory data) = withdrawal.usr.call{value: withdrawal.amount}("");
            if (success) {
                reserve -= withdrawal.amount;
            } else {
                emit WithdrawalFailed(withdrawal.usr, withdrawal.amount, data);
            }
            i += 1;

If the withdrawal.usr address is a contract then the contract is called and the relevant logic in the fallback is implemented. If the fallback has a bug or intermittent issue, the call will fail. The withdrawal logic above calmly handles the failure, emits an event and increments the queue counter. This is bad because the user has already burnt their vUSD and now their withdrawal is in the queue at a position lower than the queue counter. There is now no way for the user to complete their withdrawal.

Below is a diff to the existing test suite that demonstrates this issue. It can be executed with forge test -vvv --match-path test/foundry/VUSDWithReceive.t.sol:

diff --git a/hubble-protocol/test/foundry/BuggyContract.sol b/hubble-protocol/test/foundry/BuggyContract.sol
new file mode 100644
index 0000000..a0de2fa
--- /dev/null
+++ b/hubble-protocol/test/foundry/BuggyContract.sol
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity 0.8.9;
+
+import "../../contracts/Interfaces.sol";
+
+contract BuggyContract {
+    // Fallback with a bug
+    fallback() payable external {
+        revert();
+    }
+
+    function qWithdrawal(address husd, uint256 amount) public {
+        IVUSD(husd).withdraw(amount);
+    }
+}
diff --git a/hubble-protocol/test/foundry/VUSDWithReceive.t.sol b/hubble-protocol/test/foundry/VUSDWithReceive.t.sol
index 7152af4..51d1911 100644
--- a/hubble-protocol/test/foundry/VUSDWithReceive.t.sol
+++ b/hubble-protocol/test/foundry/VUSDWithReceive.t.sol
@@ -1,36 +1,47 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity 0.8.9;
 import "./Utils.sol";
+import "./BuggyContract.sol";
 
 contract VUSDWithReceiveTest is Utils {
     event WithdrawalFailed(address indexed trader, uint amount, bytes data);
 
+    BuggyContract buggyContract;
+
+
     function setUp() public {
         setupContracts();
+
+        // Deploy malicious contract
+        buggyContract = new BuggyContract();
     }
 
     function testWithdrawWithReceive(uint128 amount) public {
         vm.assume(amount >= 5e6);
-        // mint vusd for this contract
-        mintVusd(address(this), amount);
-        // alice and bob also mint vusd
-        mintVusd(alice, amount);
-        mintVusd(bob, amount);
-
-        // withdraw husd
-        husd.withdraw(amount); // first withdraw in the array
-        vm.prank(alice);
-        husd.withdraw(amount);
-        vm.prank(bob);
-        husd.withdraw(amount);
-
-        assertEq(husd.withdrawalQLength(), 3);
+        mintVusd(address(buggyContract), amount);
+
+        buggyContract.qWithdrawal(address(husd), amount);
+
+        // Let's create some other users who also want to withdraw
+        // In this case we create 1 other user who also want to withdraw and join the queue
+        uint160 addressCounter = 1000;
+        for (uint160 i = 0; i < 1; i++) {
+            mintVusd(address(addressCounter + i), amount);
+            vm.prank(address(addressCounter + i));
+            husd.withdraw(amount);
+        }
+
+        assertEq(husd.withdrawalQLength(), 2);
         assertEq(husd.start(), 0);
 
         husd.processWithdrawals();
 
-        assertEq(husd.withdrawalQLength(), 3);
-        assertEq(husd.start(), 3);
+        assertEq(husd.withdrawalQLength(), 2);
+        // The withdrawal counter has been incremented
+        assertEq(husd.start(), 2);
+        // But the buggy contract hasn't received any funds and doesn't have any vUSD now either
+        assertEq(address(buggyContract).balance, 0);
+        assertEq(husd.balanceOf(address(buggyContract)), 0);
     }
 
     receive() payable external {

Impact

Where a smart contract is trying to withdraw from the VUSD contract, if the smart contract has an intermittent issue or bug, their funds are lost forever.

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/VUSD.sol#L75-L84
https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/VUSD.sol#L111-L112

Tool used

Manual Review

Recommendation

Assuming the same 2 step withdrawal with a global queue, then a failed withdrawal should also mint the corresponding vUSD back to the desired recipient to allow them to retry the withdrawal.

However I would recommend moving away from the global queue to a per-user queue or to a 1 step withdrawal mechanism instead (since there is another issue regarding a malicious user blocking withdrawals).

Duplicate of #162