Wrong implementation of `CreditLimitByMedian.sol#getLockedAmount()` will lock a much bigger total amount of staked tokens than expected

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-union/blob/4176c366986e6d1a6b3f6ec0079ba547b040ac0f/contracts/user/CreditLimitByMedian.sol#L27-L63

function getLockedAmount(
    LockedInfo[] memory array,
    address account,
    uint256 amount,
    bool isIncrease
) public pure override returns (uint256) {
    if (array.length == 0) return 0;

    uint256 newLockedAmount;
    if (isIncrease) {
        for (uint256 i = 0; i < array.length; i++) {
            uint256 remainingVouchingAmount;
            if (array[i].vouchingAmount > array[i].lockedAmount) {
                remainingVouchingAmount = array[i].vouchingAmount - array[i].lockedAmount;
            } else {
                remainingVouchingAmount = 0;
            }

            if (remainingVouchingAmount > array[i].availableStakingAmount) {
                if (array[i].availableStakingAmount > amount) {
                    newLockedAmount = array[i].lockedAmount + amount;
                } else {
                    newLockedAmount = array[i].lockedAmount + array[i].availableStakingAmount;
                }
            } else {
                if (remainingVouchingAmount > amount) {
                    newLockedAmount = array[i].lockedAmount + amount;
                } else {
                    newLockedAmount = array[i].lockedAmount + remainingVouchingAmount;
                }
            }

            if (account == array[i].staker) {
                return newLockedAmount;
            }
        }
    } else {
    ...

getLockedAmount() is used by UserManager.sol#updateLockedData() to update locked amounts.

The current implementation is wrong and locks every staker for the amount of the borrowed amount or all the vouchingAmount if the vouchingAmount is smaller than the borrowed amount in CreditLimitByMedian model.

PoC

• 10 stakers each give 100 of vouchingAmount to Alice;
• Alice borrows 100;

The protocol will now lock 100 of each of the 10 stakers, making the total locked amount being 1000.

[kingjacob]

This is as designed.

[GalloDaSballo]

With the evidence shown and the comment of the sponsor, it seems to me like this is a bad idea, the protocol will lock funds at a rate of N * X, where X was the original amount borrowed and N is the number of stakers that are covering for it.

This seems to be a surefire way for griefing, DOS attacks, and potentially to block other people funds in the protocol

I would highly recommend the sponsor to consider the possibility that this behaviour can be used against the users of the protocol and can potentially kill the protocol in it's infancy

[kingjacob]

While creditlimitbymedian seems inefficient from a capital locked per loans outstanding view, it is a valid if agressive model. We’ve run agent simulation on both it and sumoftrust and theres tradeoffs with both.

For clarity of whats deployed, we’ll be deleting creditlimitbymedian from the repo but it is a valid implementation.

[kingjacob]

@GalloDaSballo this issue id argue is invalid as the implementation is correct and locks funds as instructed by creditlimitbymedian. Which is as designed.

Locking > $M in underwriting per $M in borrowed funds is also not a “loss of funds” its common practice in credit markets, and might actually end up be required if default rates are high enough. And in the above model, the only one who can lock funds is the person the user chose to give the right to borrow (and lock the funds). Which I’d argue is distinct from an actual grief or DoS. (Unless you had something else in mind?)

But all thats a bit offtopic, as this issue is reporting a wrong implementation, not an inefficient underwriting model.

[GalloDaSballo]

@kingjacob For the sake of understarding, let's assume I convince 10 people to each put 1MLN so I can borrow 1 MLN. If I then run away with the money and default, their 10 MLN would get locked.

What would happen after?

[kingjacob]

@GalloDaSballo nothing. Stake stays locked earning interest which slowly fills the whole left by the default. Unless one of the stakers calls writeoffdebt.

We dont solve the problem of what if someone trusts someone they shouldnt. Email me at jacob@union.finance, can hop on a call to explain in more detail.

[GalloDaSballo]

Me and the sponsor have scheduled a clarification call, the conversation should help clarify our different opinions

Before the call I'd like to record my current opinion:

• The system does what the sponsor designed (locks funds on default)
• To my understanding the system will lock N times the tokens necessary (again by design)
• By my experience in DeFi this means that potentially a lot of people can get locked
• There is a technicality with the findings that says "Wrong Implementation", the implementation is as the sponsor specified, however, for a myriad of reasons, wrong is a synonym with "fallacious" / "errouneous", hence my interpretation of the finding at this time is that it's not a warning for the code not being to spec, but rather the code allowing for a risky situation (tons of people with funds locked)

Ref: Google's synonims for wrong

[GalloDaSballo]

Given the specifics of this risk I am conflicted between a High (can lock arbitrary funds for an arbitrary amount of people) and Medium (they agreed to the dynamic, the time being locked is proportional to yield, higher yield = less time) severity finding, however believe it is correct for me to talk with the sponsor to hear their side of the story

[GalloDaSballo]

After the conversation and the clarifications from the sponsor we both agree that the finding is of medium severity. Allowing someone to borrow is a situation closer to giving your allowance, more of a feature than a risk. However, locking a disproportional amount of people on default can be problematic and I think there should be a cap on that.

The sponsor will mitigate by removing CrediLimitByMedian from the codebase

[GalloDaSballo]

For the sake of transparency, we have recorded the call here https://us02web.zoom.us/rec/share/kx789PdETc3NqJ7J9gy1tNHazJAmYufFfwKD-ob_Ct6akpnC-sZPp84cLozLOlpm.khrAW4nfFCPxJZCx Passcode: 2A07bsS@

[GalloDaSballo]

I've re-rated the severity of the issue given the specifics of the risk, as per CodeArena docs: 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.