ArmedGoose - amoMinterBorrow is insecure in its current state, might disrupt the protocol if used

[sherlock-admin]

ArmedGoose

medium

amoMinterBorrow is insecure in its current state, might disrupt the protocol if used

Summary

Function amoMinterBorrow implemented in LibUbiquityPool despite being a whitelist-only called function, poses significant risk to the protocol. It simply allows the whitelisted address (contract or EOA) to transfer an arbitrary amount of collateral out of UbiquityPool, without accounting for amounts reserved for fees, redemption and without requirement to return the funds, or collateralize the borrowed amount and without any timely return requirement. This is too lenient even for a supposedly "TRUSTED" entity, moreover, freeze of user funds may happen accidentally, if collateral to be redeemed is borrowed in the same time, so contract will not have enough balance to cover redemptions.

Vulnerability Detail

The implementation of amoMinterBorrow is pretty straightforward, once an entity is added as an AMO minter borrower, it just checks if there's no pause and if minter's collateral is currently active, and allows to transfer away arbitrary amount of the collateral from the Ubiquity contract's balance.

Even if rule out the worst case scenario, when after an AMO compromise all collateral can be taken away, the sure thing is that there is no separation of accounting of fees and redeemCollateralBalances, so in a day-to-day operations, some users might not be able to redeem their funds until AMO returns the balance. Simply, the protocol has no way to recognize if the current borrow will put redemptions at risk or not, because it can just use the whole contract balance.

Impact

• Users funds temporarily locked if AMO borrows the balance that users want to currently redeem. This cannot be defended against, since AMO do not know how much funds are currently to be redeemed.
• The same may happen to fees, which are not accounted, but can be simply borrowed away
• In worst case, there is no security against not returning funds, any hack or malfunction of external protocol (although this point may be disputable since protocol excludes "external integration risks" - however this one is significant)

Code Snippet

    function amoMinterBorrow(uint256 collateralAmount) internal onlyAmoMinter {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        // checks the collateral index of the minter as an additional safety check
        uint256 minterCollateralIndex = IDollarAmoMinter(msg.sender)
            .collateralIndex();

        // checks to see if borrowing is paused
        require(
            poolStorage.isBorrowPaused[minterCollateralIndex] == false,
            "Borrowing is paused"
        );

        // ensure collateral is enabled
        require(
            poolStorage.isCollateralEnabled[
                poolStorage.collateralAddresses[minterCollateralIndex]
            ],
            "Collateral disabled"
        );

        // transfer
        IERC20(poolStorage.collateralAddresses[minterCollateralIndex])
            .safeTransfer(msg.sender, collateralAmount); //@audit whole balance can be taken away - no return terms / countermeasures against abuse, also no accounting.
    }

Tool used

Manual Review

Recommendation

• Introduce any accounting logic, which determines what part of current contract balance is available for AMO borrowings and what part can be liquid so users can mint/redeem without risk of being locked out
• Consider an incentive or restriction on return terms, e.g. time-based return, or requirement to collateralize the borrows unless the AMO contract is of known structure and has predictable behavior (so its known when and on what terms the collateral will return)

Duplicate of #1