sherlock-audit / 2023-04-hubble-exchange-judging

7 stars 6 forks source link

0xpinky - VUSD.sol : success state is not checked for `call` #230

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

0xpinky

high

VUSD.sol : success state is not checked for call

Summary

during processWithdrawals, contract uses the call codes to transfer the usdc to user.

The return status is not checked.

Vulnerability Detail

function processWithdrawals() external override whenNotPaused nonReentrant {
    uint reserve = address(this).balance;
    require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
    uint i = start;
    while (i < withdrawals.length && (i - start) < maxWithdrawalProcesses) {
        Withdrawal memory withdrawal = withdrawals[i];
        if (reserve < withdrawal.amount) {
            break;
        }

        (bool success, bytes memory data) = withdrawal.usr.call{value: withdrawal.amount}(""); -------------->> audit call code
        if (success) { ------------------------------------------------------------->> not checked for failure status.
            reserve -= withdrawal.amount;
        } else {
            emit WithdrawalFailed(withdrawal.usr, withdrawal.amount, data);
        }
        i += 1;
    }
    // re-entracy not possible, hence can update `start` at the end
    start = i;
}

in above code block, processWithdrawals uses the call method to transfer the usdc to user.

It check whether the transfer is successful or not using the return status. If the trasnfer is successful only it updates the reserve but for failure case, it is emitting the event which will not get anything to user.

There are no other function implementation to compensate the user to whom the call code failed.

Impact

Loss of fund to user.

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/VUSD.sol#L65-L85

Tool used

Manual Review

Recommendation

I think, contract wants to continue the transaction even if the return status is failure. If that is case, we suggest to add following remdey.

  1. use separate map to store the failed data.
  2. add function to use the above map (withdrawal data)
  3. allow user to call this new function.

Duplicate of #162