hyh - repayBorrow calls wrong frozen info update for overdue repayments

[sherlock-admin]

hyh

high

repayBorrow calls wrong frozen info update for overdue repayments

Summary

UToken's repayBorrow() calls frozen info update with wrong account, that end up not doing anything meaningful in the majority of cases, which leaves the stakers' account frozen info not updated, which leads to Comptroller accounting with the stale figures via debtWriteOff().

Vulnerability Detail

User facing UToken's repayBorrow() calling _repayBorrowFresh() end up calling IUserManager(userManager).updateFrozenInfo(borrower, 0) to update frozen coin age data. However in the most cases it is pointless to call this function for the borrower as most borrowers do not have issued loans and the state of these issued loans didn't changed when this account paid own debt.

I.e. IUserManager(userManager).updateFrozenInfo(staker, 0) is a proper call and it should be done for all borrower's stakers to reflect the changed state of their issued loans, which got healthier as borrower paid full interest or even made a prepayment with repayBorrow().

This way repayBorrow() do not update borrower's stakers frozen coin age info and this way direct UserManager's batchUpdateFrozenInfo() and Comptroller's withdrawRewards() remain the only venues for lender's account total health update.

Impact

Given that stakers are unaware of this lacking update, and assuming that frozen info update is taking place on each repayment, so frozen info is up to date, and not calling manually batchUpdateFrozenInfo() before each debtWriteOff(), they invoke Comptroller accounting update with stale totalFrozen in totalStaked - totalFrozen, diminishing effectiveAmount in Comptroller’s inflation calculations.

As this is UNION reward miscalculation without low probability prerequisites (i.e. base functionality not working as intended), setting the severity to be high.

Code Snippet

When a borrower is overdue, repayBorrow() -> _repayBorrowFresh() calls for

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/market/UToken.sol#L573-L626

    function _repayBorrowFresh(
        address payer,
        address borrower,
        uint256 amount
    ) internal {
        if (!accrueInterest()) revert AccrueInterestFailed();

        uint256 interest = calculatingInterest(borrower);
        uint256 borrowedAmount = borrowBalanceStoredInternal(borrower);
        uint256 repayAmount = amount > borrowedAmount ? borrowedAmount : amount;
        if (repayAmount == 0) revert AmountZero();

        uint256 toReserveAmount;
        uint256 toRedeemableAmount;

        if (repayAmount >= interest) {
            // If the repayment amount is greater than the interest (min payment)
            bool isOverdue = checkIsOverdue(borrower);

            // Interest is split between the reserves and the uToken minters based on
            // the reserveFactorMantissa When set to WAD all the interest is paid to teh reserves.
            // any interest that isn't sent to the reserves is added to the redeemable amount
            // and can be redeemed by uToken minters.
            toReserveAmount = (interest * reserveFactorMantissa) / WAD;
            toRedeemableAmount = interest - toReserveAmount;

            // Update the total borrows to reduce by the amount of principal that has
            // been paid off
            totalBorrows -= (repayAmount - interest);

            // Update the account borrows to reflect the repayment
            accountBorrows[borrower].principal = borrowedAmount - repayAmount;
            accountBorrows[borrower].interest = 0;

            if (getBorrowed(borrower) == 0) {
                // If the principal is now 0 we can reset the last repaid block to 0.
                // which indicates that the borrower has no outstanding loans.
                accountBorrows[borrower].lastRepay = 0;
            } else {
                // Save the current block number as last repaid
                accountBorrows[borrower].lastRepay = getBlockNumber();
            }

            // Call update locked on the userManager to lock this borrowers stakers. This function
            // will revert if the account does not have enough vouchers to cover the repay amount. ie
            // the borrower is trying to repay more than is locked (owed)
            IUserManager(userManager).updateLocked(borrower, uint96(repayAmount - interest), false);

            if (isOverdue) {
                // For borrowers that are paying back overdue balances we need to update their
                // frozen balance and the global total frozen balance on the UserManager
                IUserManager(userManager).updateFrozenInfo(borrower, 0);
            }
        }

However, updateFrozenInfo() -> _updateFrozen() -> getFrozenInfo() aims to update overdue info from lender perspective, cycling through vouchees array of the issued loans:

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L438-L466

    function getFrozenInfo(address staker, uint256 pastBlocks)
        public
        view
        returns (uint256 memberTotalFrozen, uint256 memberFrozenCoinAge)
    {
        uint256 overdueBlocks = uToken.overdueBlocks();
        uint256 voucheesLength = vouchees[staker].length;
        // Loop through all of the stakers vouchees sum their total
        // locked balance and sum their total memberFrozenCoinAge
        for (uint256 i = 0; i < voucheesLength; i++) {
            // Get the vouchee record and look up the borrowers voucher record
            // to get the locked amount and lastUpdate block number
            Vouchee memory vouchee = vouchees[staker][i];
            Vouch memory vouch = vouchers[vouchee.borrower][vouchee.voucherIndex];

            uint256 lastUpdated = vouch.lastUpdated;
            uint256 diff = block.number - lastUpdated;

            if (overdueBlocks < diff) {
                uint96 locked = vouch.locked;
                memberTotalFrozen += locked;
                if (pastBlocks >= diff) {
                    memberFrozenCoinAge += (locked * diff);
                } else {
                    memberFrozenCoinAge += (locked * pastBlocks);
                }
            }
        }
    }

A borrower can also be a staker, but the intersection is reasonably small, i.e. calling IUserManager(userManager).updateFrozenInfo(borrower, 0) will be a noop in the vast majority of cases.

It is also called in Comptroller via user-facing withdrawRewards() invoking _getUserInfo() that calls updateFrozenInfo():

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/token/Comptroller.sol#L231-L245

    function withdrawRewards(address account, address token)
        external
        override
        whenNotPaused
        onlyUserManager(token)
        returns (uint256)
    {
        IUserManager userManager = _getUserManager(token);

        // Lookup account state from UserManager
        (
            UserManagerAccountState memory userManagerAccountState,
            Info memory userInfo,
            uint256 pastBlocks
        ) = _getUserInfo(userManager, account, token, 0);

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/token/Comptroller.sol#L361-L384

    function _getUserInfo(
        IUserManager userManager,
        address account,
        address token,
        uint256 futureBlocks
    )
        internal
        returns (
            UserManagerAccountState memory,
            Info memory,
            uint256
        )
    {
        Info memory userInfo = users[account][token];
        uint256 lastUpdatedBlock = userInfo.updatedBlock;
        if (block.number < lastUpdatedBlock) {
            lastUpdatedBlock = block.number;
        }

        uint256 pastBlocks = block.number - lastUpdatedBlock + futureBlocks;

        UserManagerAccountState memory userManagerAccountState;
        (userManagerAccountState.totalFrozen, userManagerAccountState.pastBlocksFrozenCoinAge) = userManager
            .updateFrozenInfo(account, pastBlocks);

withdrawRewards() is a staker reward withdrawal function.

The impact of the frozen info not being updated on borrower’s repay is comptroller.updateTotalStaked proceeds with the stale totalFrozen figure:

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L726-L788

    function debtWriteOff(
        address staker,
        address borrower,
        uint96 amount
    ) external {
        if (amount == 0) revert AmountZero();
        uint256 overdueBlocks = uToken.overdueBlocks();
        uint256 lastRepay = uToken.getLastRepay(borrower);

        // This function is only callable by the public if the loan is overdue by
        // overdue blocks + maxOverdueBlocks. This stops the system being left with
        // debt that is overdue indefinitely and no ability to do anything about it.
        if (block.number <= lastRepay + overdueBlocks + maxOverdueBlocks) {
            if (staker != msg.sender) revert AuthFailed();
        }

        Index memory index = voucherIndexes[borrower][staker];
        if (!index.isSet) revert VoucherNotFound();
        Vouch storage vouch = vouchers[borrower][index.idx];

        if (amount > vouch.locked) revert ExceedsLocked();

        // update staker staked amount
        stakers[staker].stakedAmount -= amount;
        stakers[staker].locked -= amount;
        totalStaked -= amount;

        // update vouch trust amount
        vouch.trust -= amount;
        vouch.locked -= amount;

        // Update total frozen and member frozen. We don't want to move th
        // burden of calling updateFrozenInfo into this function as it is quite
        // gas intensive. Instead we just want to remove the amount that was
        // frozen which is now being written off. However, it is possible that
        // member frozen has not been updated prior to calling debtWriteOff and
        // the amount being written off could be greater than the amount frozen.
        // To avoid an underflow here we need to check this condition
        uint256 stakerFrozen = memberFrozen[staker];
        if (amount > stakerFrozen) {
            // The amount being written off is more than the amount that has
            // been previously frozen for this staker. Reset their frozen stake
            // to zero and adjust totalFrozen
            memberFrozen[staker] = 0;
            totalFrozen -= stakerFrozen;
        } else {
            totalFrozen -= amount;
            memberFrozen[staker] -= amount;
        }

        if (vouch.trust == 0) {
            cancelVouch(staker, borrower);
        }

        // Notify the AssetManager and the UToken market of the debt write off
        // so they can adjust their balances accordingly
        IAssetManager(assetManager).debtWriteOff(stakingToken, uint256(amount));
        uToken.debtWriteOff(borrower, uint256(amount));

        comptroller.updateTotalStaked(stakingToken, totalStaked - totalFrozen);

        emit LogDebtWriteOff(msg.sender, borrower, uint256(amount));
    }

Tool used

Manual Review

Recommendation

Consider calling it for borrower's staker instead:

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/market/UToken.sol#L621-L625

            if (isOverdue) {
                // For borrowers that are paying back overdue balances we need to update their
                // frozen balance and the global total frozen balance on the UserManager
-               IUserManager(userManager).updateFrozenInfo(borrower, 0);
+		... // cycle all stakers of the borrower
+                  IUserManager(userManager).updateFrozenInfo(staker, 0);
            }

An alternative to this is to call updateFrozenInfo() in updateLocked() as it implements closely related logic.

[kingjacob]

This is as designed, The contract assumes a minimal level of activity or a keeper to keep reward calcs up to date.

[dmitriia]

It's not about updating one staker or all of them, it's updating staker, not the borrower: calling IUserManager(userManager).updateFrozenInfo(borrower, 0) is just pointless, most borrowers will not have stakes of their own, and those stakes, if exist, aren't updated on repayBorrow.

In other words:

• Bob opens trust for Alice
• Alice borrows, then repays
• updateFrozenInfo is called for Alice, while she doesn't have any stakes and frozen info is irrelevant for her
• updateFrozenInfo should be called for Bob
• ideally for all Alice stakers, but it's design question indeed, the issue is about calling for Bob instead of Alice

[kingjacob]

@dmitriia gotcha, youre probably right on this.

[GeraldHost]

unioncredit/union-v2-contracts#79