code-423n4 / 2021-10-union-findings

0 stars 0 forks source link

Wrong implementation of `CreditLimitByMedian.sol#getLockedAmount()` makes it unable to unlock `lockedAmount` in `CreditLimitByMedian` model #80

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

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

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) {
        ...
    } else {
        for (uint256 i = 0; i < array.length; i++) {
            if (array[i].lockedAmount > amount) {
                newLockedAmount = array[i].lockedAmount - 1;
            } else {
                newLockedAmount = 0;
            }

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

    return 0;
}

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

Based on the context, at L66, newLockedAmount = array[i].lockedAmount - 1; should be newLockedAmount = array[i].lockedAmount - amount;.

The current implementation is wrong and makes it impossible to unlock lockedAmount in CreditLimitByMedian model.

Recommendation

Change to:

newLockedAmount = array[i].lockedAmount - amount;

GalloDaSballo commented 2 years ago

The warden identified a mistake in the accounting that would make it impossible to unlock funds, mitigation seems to be straightfoward