sherlock-audit / 2023-02-carapace-judging

2 stars 0 forks source link

__141345__ - DoS in `_calculateClaimableAmount()` #283

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

141345

medium

DoS in _calculateClaimableAmount()

Summary

The lockedCapitals array could grow unbounded and result in DoS when trying to Claim Unlocked Capital.

Vulnerability Detail

When adding new item to lockedCapitals array, there is no limit on how many items can be created.

File: contracts/core/DefaultStateManager.sol
390:   function _moveFromActiveToLockedState(

401:     /// step 2: create and store an instance of locked capital for the lending pool
402:     poolState.lockedCapitals[_lendingPool].push(
403:       LockedCapital({
404:         snapshotId: _snapshotId,
405:         amount: _lockedCapital,
406:         locked: true
407:       })
408:     );

As a result, the lockedCapitals array could grow quite large, the transaction’s gas cost could exceed the block gas limit and make it impossible to call this function _calculateClaimableAmount() at all. Since inside the for loop, there are storage load and function calls, the gas cost of these are relatively high.

453:   function _calculateClaimableAmount(

476:     uint256 _length = lockedCapitals.length;
477:     for (uint256 _index = 0; _index < _length; ) {
478:       LockedCapital storage lockedCapital = lockedCapitals[_index];

Impact

The pool could fail to call function calculateAndClaimUnlockedCapital(). User fund could get locked and unable to claim.

Code Snippet

https://github.com/sherlock-audit/2023-02-carapace/blob/main/contracts/core/DefaultStateManager.sol#L476-L520

Tool used

Manual Review

Recommendation

Consider introducing a reasonable upper limit based on block gas limits. For large size lockedCapitals array, the handling of each lockedCapital struct can be break into separate parts with different function calls, and combine the results at last in another function.

Duplicate of #113