390.md

GimelSec

medium

PerpDepositor.withdrawInsurance should call vault.getFreeCollateral first

Summary

When withdrawing collaterals from the perp vault, one can withdraw the amount up to its freeCollateral. So it should check the amount of freeCollateralbefore withdrawing in PerpDepositor.withdrawInsurance. Otherwise, it would revert due to (https://github.com/perpetual-protocol/perp-curie-contract/blob/main/contracts/Vault.sol#L677)

Vulnerability Detail

PerpDepositor.withdrawInsurance is a simple function which withdraws insurance from the perp vault.

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L208

    function withdrawInsurance(uint256 amount, address to)
        external
        nonReentrant
        onlyOwner
    {
        if (amount == 0) {
            revert ZeroAmount();
        }

        insuranceDeposited -= amount;

        vault.withdraw(insuranceToken(), amount);
        IERC20(insuranceToken()).transfer(to, amount);

        emit InsuranceWithdrawn(msg.sender, to, amount);
    }

But the perp vault would check how much collateral the user can withdraw. It would revert if the amount is higher than the amount of free collateral. https://github.com/perpetual-protocol/perp-curie-contract/blob/main/contracts/Vault.sol#L677

    function withdraw(address token, uint256 amount)
        external
        override
        whenNotPaused
        nonReentrant
        onlySettlementOrCollateralToken(token)
    {
        // input requirement checks:
        //   token: here
        //   amount: in _settleAndDecreaseBalance()

        address to = _msgSender();
        _withdraw(to, token, amount);
    }

    function _settleAndDecreaseBalance(
        address to,
        address token,
        uint256 amount
    ) internal {
        …
        uint256 freeCollateral = getFreeCollateralByToken(to, token);
        // V_NEFC: not enough freeCollateral
        require(freeCollateral >= amount, "V_NEFC");

       …
    }

Impact

PerpDepositor.withdrawInsurance can only be called by the owner. Which means that It would be called through governance proposals. From creation to execution, a proposal takes some time. If a governance proposal has been created, voted on and passed by a community vote, it should be executed as desired.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L208

Tool used

Manual Review

Recommendation

Call vault.getFreeCollateral() before vault.withdraw(insuranceToken(), amount) to check how much collateral it can withdraw.

If UXD wants to withdraw the insurance as much as possible. the function could be modified like the following:

    function withdrawInsurance(uint256 amount, address to)
        external
        nonReentrant
        onlyOwner
    {
        if (amount == 0) {
            revert ZeroAmount();
        }
        uint256 freeCollateral = vault.getFreeCollateral(address(this));
        uint256 newAmount = amount > freeCollateral  ? freeCollateral : amount; 
        insuranceDeposited -= newAmount;

        vault.withdraw(insuranceToken(), newAmount);
        IERC20(insuranceToken()).transfer(to, newAmount);

        emit InsuranceWithdrawn(msg.sender, to, newAmount);
    }