UserManager: updateLockedData() doesn't check that the amount is actually locked.

[code423n4]

Handle

itsmeSTYJ

Vulnerability details

Impact

The function updateLockedData() does not actually check if the amount required to be locked is actually locked.

Proof of Concept

Same solution as my other high issue. I've added comments where relevant.

function updateLockedData(
  address borrower,
  uint256 amount,
  bool isBorrow
) external override onlyMarketOrAdmin {
    TrustInfo memory trustInfo;
    trustInfo.stakerAddresses = members[borrower].creditLine.stakerAddresses;

    ICreditLimitModel.LockedInfo[] memory lockedInfoList = new ICreditLimitModel.LockedInfo[](
        trustInfo.stakerAddresses.length
    );

    uint256 numStakers; // added this
    uint256 totalAvailableVouch; // added this

    for (uint256 i = 0; i < trustInfo.stakerAddresses.length; i++) {
        ICreditLimitModel.LockedInfo memory lockedInfo;

        trustInfo.staker = trustInfo.stakerAddresses[i];
        trustInfo.stakingAmount = stakers[trustInfo.staker];
        trustInfo.vouchingAmount = getVouchingAmount(trustInfo.staker, borrower);
        trustInfo.totalLockedStake = getTotalLockedStake(trustInfo.staker);
        // deleted this
        // if (trustInfo.stakingAmount <= trustInfo.totalLockedStake) {
        //     trustInfo.availableStakingAmount = 0;
        // } else {
        //     trustInfo.availableStakingAmount = trustInfo.stakingAmount - trustInfo.totalLockedStake;
        // }

        if (trustInfo.stakingAmount <= trustInfo.totalLockedStake) continue; // added this, only interested in stakers with surplus staking balance
        trustInfo.availableStakingAmount = trustInfo.stakingAmount - trustInfo.totalLockedStake; // added this

        lockedInfo.lockedAmount = getLockedStake(trustInfo.staker, borrower);
        if (lockedInfo.lockedAmount <= trustInfo.vouchingAmount) continue;  // added this, only interested in stakers with surplus vouching balance

        totalAvailableVouch += trustInfo.vouchingAmount - lockedInfo.lockedAmount; // added this, to calculate sum of available vouching balance
        lockedInfo.staker = trustInfo.staker;
        lockedInfo.vouchingAmount = trustInfo.vouchingAmount;
        lockedInfo.availableStakingAmount = trustInfo.availableStakingAmount;
        lockedInfoList[i] = lockedInfo;

        numStakers += 1; // added this
    }

    require(totalAvailableVouch >= amount, "Sum of available vouch is less than amount to be locked");

    uint256 balance = amount;
    uint amountToLock;
    for(uint i = 0; i < numStakers; i ++) {
        amountToLock = lockedInfo[i].vouchingAmount - lockedInfo[i].lockedAmount // calc amount to lock
        amountToLock = amountToLock > balance ? balance : lockAmount; // handle edge case when surplus vouching balance > balance to lock since you don't want to overlock
        members[lockedInfoList[i].staker].creditLine.lockedAmount[borrower] = creditLimitModel.getLockedAmount(
            lockedInfoList,
            lockedInfoList[i].staker,
            amountToLock, // updated this
            isBorrow
        );
        balance -= amountToLock;
    }
    require(balance == 0, "critical error!"); // this revert should never fail.

    // deleted this
    // for (uint256 i = 0; i < lockedInfoList.length; i++) {
    //     members[lockedInfoList[i].staker].creditLine.lockedAmount[borrower] = creditLimitModel.getLockedAmount(
    //         lockedInfoList,
    //         lockedInfoList[i].staker,
    //         amount,
    //         isBorrow
    //     );
    // }
}

[GalloDaSballo]

Disagree with the finding, while the logic for locking can be simplified, it does update storage variables via getLockedAmount