UserManager: updateLockedData() locks more amount than required.

[code423n4]

Handle

itsmeSTYJ

Vulnerability details

Impact

The function updateLockedData() calls creditLimitModel.getLockedAmount() which is executed in a for loop with amount passed as an argument. This means that all stakers that are staking on behalf of the borrower are asked to lock amount . If I wanted to lock only 10 DAI and I had 5 stakers staking for me, all of them (if they have sufficient balance) will be asked to lock 10 DAI.

Recommended Mitigation Steps

I added my 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
    //     );
    // }
}

[kingjacob]

Not able to confirm this behavior.

[GeraldHost]

If this is in regards to the CreditLimitByMedian locking up the same amount for each user then this is the expected behaviour for that credit model. We don't actually use that credit model anymore in favour of SumOfTrust.

[GalloDaSballo]

I don't fully understand what the warden was trying to convey

In the absence of enough evidence I'll have to set this as well as 50 as invalid

Will review tomorrow to see if I can figure this out

[GalloDaSballo]

After thinking about it, and seeing other findings, this is a duplicate of #81

As in 81 the warden identified that the protocol will lock N * X funds to cover for a borrow

Duplicate of #81

[GalloDaSballo]

Since #81 was downgraded to medium, this also get's rated medium