search

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

0 stars 0 forks source link

CreditLimitByMedian: getLockedAmount() can be optimized further. #34

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

itsmeSTYJ

Vulnerability details

Impact

Gas optimisations + cleanup of code for improved readability

Proof of Concept

function getLockedAmount(
  LockedInfo memory lockedInfo, // updated this
  // address account, // deleted this
  uint256 amount,
  bool isIncrease
) public pure override returns (uint256 newLockedAmount) { // updated return value
    // if (array.length == 0) return 0; deleted this

    // uint256 newLockedAmount; // deleted this
    if (isIncrease) {
        // deleted this
        // 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;
        //     }
        // }
        uint256 remainingVouchingAmount; // added this
        uint256 toAdd; // added this
        remainingVouchingAmount = lockedInfo.vouchingAmount > lockedInfo.lockedAmount ? lockedInfo.vouchingAmount - lockedInfo.lockedAmount : 0; // added this
        toAdd = remainingVouchingAmount > lockedInfo.availableStakingAmount ? lockedInfo.availableStakingAmount : remainingVouchingAmount; // added this
        toAdd = toAdd > amount ? amount : toAdd; // added this
        newLockedAmount = lockedInfo.lockedAmount + toAdd; // added this

    } else {
        // deleted this
        // 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;
        //     }
        // }
        newLockedAmount = lockedInfo.lockedAmount > amount ? lockedInfo.lockedAmount - 1 : 0; // added this
    }

    // return 0; // deleted this
}
  1. Replaced array with single lockedInfo argument to avoid having to loop through
  2. deleted account parameter in the function signature
  3. Refactored code to make it more gas optimized (no loops) and more readable
  4. In UserManager.updateLockedData(), creditLimitModel.getLockedAmount() should be called with the following params lockedInfoList[i], amount and isBorrow.
GalloDaSballo commented 3 years ago

Agree with the refactoring as it simplifies code and should save some gas in certain cases (nested if else)