capy_ - Double accounting of `_getPortfolio` amounts

[sherlock-admin]

capy_

medium

Double accounting of _getPortfolio amounts

Summary

Double accounting of _getPortfolio amounts in function _swapOut of Unitas.sol

Vulnerability Detail

Function _swapOut of Unitas.sol as follows:

    function _swapOut(address token, address receiver, uint256 amount) internal {
        ITokenManager.TokenType tokenType = tokenManager.getTokenType(token);

        require(tokenType != ITokenManager.TokenType.Undefined);

        if (tokenType == ITokenManager.TokenType.Asset) {
            uint256 tokenReserve = _getBalance(token);
            uint256 reserveAmount = amount.min(tokenReserve - _getPortfolio(token));

            if (amount > reserveAmount) {
                uint256 collateralAmount = amount - reserveAmount;

                // Pull the collateral from insurance pool
                IInsurancePool(insurancePool).withdrawCollateral(token, collateralAmount);
            }

            _setBalance(token, tokenReserve - reserveAmount);
            IERC20(token).safeTransfer(receiver, amount);
        } else {
            IERC20Token(token).mint(receiver, amount);
        }
    }

Specifically in L386, the below formula considers reserveAmount to be the value of available reserve after reducing the amounts used in portfolio for strategic investments. i.e. if amount > reserveAmount, the different is attempted to be withdrawn from the insurancePool.

uint256 tokenReserve = _getBalance(token);
            uint256 reserveAmount = amount.min(tokenReserve - _getPortfolio(token));

            if (amount > reserveAmount) {
                uint256 collateralAmount = amount - reserveAmount;

                // Pull the collateral from insurance pool
                IInsurancePool(insurancePool).withdrawCollateral(token, collateralAmount);

However, function withdrawCollateral in Insurancepool.sol also checks the collateral amount with portfolio amounts, as shown:

  function withdrawCollateral(address token, uint256 amount) external onlyGuardianOrWithdrawer nonReentrant {
        _checkAmountPositive(amount);

        uint256 collateral = _getBalance(token);
        _require(collateral - _getPortfolio(token) >= amount, Errors.POOL_BALANCE_INSUFFICIENT);

        _setBalance(token, collateral - amount);

        IERC20(token).safeTransfer(msg.sender, amount);

        emit CollateralWithdrawn(token, msg.sender, amount);
    }

This means that portfolio amounts are accounted for twice in the checks before amounts from reserve or collateral pool are withdrawn, when it should be accounted for only once.

Impact

This may disallow swaps to USDT earlier than expected, resulting in bricked funds.

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/d5328421bea80e3b0fd4595e4eb6b732a40e421e/Unitas-Protocol/src/Unitas.sol#L379-L400

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/d5328421bea80e3b0fd4595e4eb6b732a40e421e/Unitas-Protocol/src/InsurancePool.sol#L98-L109

Tool used

Manual Review

Recommendation

Account for portfolio amounts only once.