CFSecurity - `UserManager::updateTrust` allows staker to increase their trust for another user despite being overdue on their loan

[sherlock-admin3]

CFSecurity

Medium

UserManager::updateTrust allows staker to increase their trust for another user despite being overdue on their loan

Summary

This oversight in UserManager::updateTrust undermines the core principle of Union's credit system. This vulnerability compromises the integrity of this system by allowing obviously unreliable actors to extend more credit. As the protocol relies on a network of trust, this vulnerability could create a domino effect of defaults and over-extended credit lines, potentially destabilizing the entire system.

Root Cause

In UserManager.sol there should be check for whether the staker is overdue if the new trust amount is higher than the current trust amount. The same as there is in the else block for creating a new vouch. https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/user/UserManager.sol#L595-L600

Internal pre-conditions

All involved parties must be members of the Union protocol.

External pre-conditions

No response

Attack Path

1. Staker updates trust for borrower to 100 DAI.
2. Staker borrows 100 DAI against extended trust from another user.
3. Overdue time passes (30 days currently) without staker paying on their loan.
4. Staker updates trust for borrower to 200 DAI.
5. Borrower borrows 200 DAI against the unreliable trust.

Impact

This vulnerability poses a significant threat to the integrity of the protocol's its trust-based credit system. By allowing users to increase credit lines even when overdue on loans, it undermines the core principles of responsible lending and could lead to a cascade of defaults.

PoC

Add the following lines to the vouching.ts hardhat integration test setup:

    context("Adjusting trust", () => {
        let staker: Signer;
        let borrower: Signer;
+        let staker2: Signer;

        before(async () => {
            await beforeContext();
            staker = signers[0];
            borrower = signers[1];
+            staker2 = signers[2];

And the add and run the following test:

        it("can increase trust amount when overdue", async () => {
            const newTrustAmount = trustAmount.div(2);
            await helpers.stake(trustAmount, staker2);
            await helpers.updateTrust(staker, borrower, newTrustAmount);
            const [vouchAmountBefore] = await helpers.getVouchingAmounts(borrower, staker);
            console.log("Init vouch amount:", vouchAmountBefore.toString());

            await helpers.updateTrust(staker2, staker, newTrustAmount);
            const [creditLimit] = await helpers.getCreditLimits(staker);
            console.log("Credit limit staker:", creditLimit.toString());
            helpers.borrow(staker, creditLimit);
            const time = ethers.BigNumber.from(getConfig("default").uToken.overdueTime);
            const timeSkip = time.add(100)
            await ethers.provider.send("evm_increaseTime", [timeSkip.toNumber()])

            await helpers.updateTrust(staker, borrower, trustAmount);
            const [vouchAmountAfter] = await helpers.getVouchingAmounts(borrower, staker);
            console.log("After vouch amount:", vouchAmountAfter.toString());

            const [creditLimitBorrower] = await helpers.getCreditLimits(borrower);
            console.log("Credit limit borrower:", creditLimitBorrower.toString());
            console.log("Amount borrowing:", (trustAmount.mul(900).div(1000)).toString());
            await helpers.borrow(borrower, trustAmount.mul(900).div(1000));

            expect(vouchAmountBefore).eq(newTrustAmount);
            expect(vouchAmountAfter).eq(trustAmount);
        });

Mitigation

Simply add the following check when modifying trust amount in UserManager.sol::updateTrust

        if (index.isSet) {
            // Update existing record checking that the new trust amount is
            // not less than the amount of stake currently locked by the borrower
            Vouch storage vouch = _vouchers[borrower][index.idx];
            if (actualTrustAmount < vouch.locked) revert TrustAmountLtLocked();
+           if (actualTrustAmount > vouch.trust && uToken.checkIsOverdue(staker)) revert VouchWhenOverdue();
            vouch.trust = actualTrustAmount;

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Overdue check meant to be only in the else block

[cholakovvv]

Escalate

There is functionally no difference between a user creating a new Vouch and increasing their trust for an existing Vouch. In both of those circumstances the lender is an unreliable actor who is extending credit despite being unable to satisfy their own obligations which potentially creates an insolvent position in the protocol. The fact that there is indeed an overdue check in the else should serve as a confirmation that this issue is valid as it shows that extending credit for other members when overdue is a highly risky and undesirable act. And this is why the lack of an overdue check in the if block is a clear oversight that should be considered a valid issue.

[sherlock-admin3]

Escalate

There is functionally no difference between a user creating a new Vouch and increasing their trust for an existing Vouch. In both of those circumstances the lender is an unreliable actor who is extending credit despite being unable to satisfy their own obligations which potentially creates an insolvent position in the protocol. The fact that there is indeed an overdue check in the else should serve as a confirmation that this issue is valid as it shows that extending credit for other members when overdue is a highly risky and undesirable act. And this is why the lack of an overdue check in the if block is a clear oversight that should be considered a valid issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

Will have to let the sponsors decide if this is an intended or a flawed design. If the latter is being vouched for, uToken.checkIsOverdue(staker)) should be checked before the if/else clause then.

[NGK02]

@mystery0x Let me clarify, it makes sense if a user is overdue for them to decrease their (not locked) existing trust for other users as this will remove any unreliable credit they are extending, thus i don't think it makes sense to put the uToken.checkIsOverdue(staker)) check before the if/else clause. Instead the user should be prevented from increasing their existing trust, with that in mind please refer to the mitigation section. Thank you for the response.

[WangSecurity]

Firstly, thank you CFSecurity for using the new templates, love to see it.

Secondly, I'll also consult with the sponsor if this is intended or not, I agree it's suboptimal, but as I understand there's no direct loss of funds here?

[NGK02]

No there is no direct loss of funds. It does however allow for insolvent positions to be created by unreliable lenders. On a side note thank you for your time and working with the new templates has been smooth.

[WangSecurity]

Further considering this issue, I believe it's actually a design recommendation. While I believe it can lead to problems and unreliable users getting more trust than they deserve, but I believe it's user's responsibility to decide whom to give the trust to.

Hence, I believe it's low severity and a design recommendation. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• cholakovvv: rejected

[maxweng]

Further considering this issue, I believe it's actually a design recommendation. While I believe it can lead to problems and unreliable users getting more trust than they deserve, but I believe it's user's responsibility to decide whom to give the trust to.

Hence, I believe it's low severity and a design recommendation. Planning to reject the escalation and leave the issue as it is.

Yes, this is our intended design. While users can vouch for an arbitrary amount, we check how much effective vouches are available when someone borrows. We did that because a staker/voucher can vouch for multiple borrowers at the same time and they all share the same credit from that staker. So, it's much easier to have the staker to manually set the trust amount, and the contracts calculate the actual borrower can borrow on the fly.