Topmark - Denial of Service When the Excess Value of from DeltaAsset is enough to handle Subsequent Asset exposure

[sherlock-admin]

Topmark

high

Denial of Service When the Excess Value of from DeltaAsset is enough to handle Subsequent Asset exposure

Summary

Denial of Service When the Excess Value of from DeltaAsset is enough to handle Subsequent Asset exposure in the AbstractDerivedAM.sol contract

Vulnerability Detail

   function _getAndUpdateExposureAsset(address creditor, bytes32 assetKey, int256 deltaAsset)
        internal
        returns (uint256 exposureAsset)
    {
        // Update exposureAssetLast.
        if (deltaAsset > 0) {
            exposureAsset = lastExposuresAsset[creditor][assetKey].lastExposureAsset + uint256(deltaAsset);
        } else {
            uint256 exposureAssetLast = lastExposuresAsset[creditor][assetKey].lastExposureAsset;
   >>>         exposureAsset = exposureAssetLast > uint256(-deltaAsset) ? exposureAssetLast - uint256(-deltaAsset) : 0;
        }
>>>        lastExposuresAsset[creditor][assetKey].lastExposureAsset = SafeCastLib.safeCastTo112(exposureAsset);
    }

The code noted above shows how asset exposure is gotten and handled through the _getAndUpdateExposureAsset(...) function, the point of interest as noted in the code above is the interaction between exposureAssetLast and negative multiplier of deltaAsset, whenever exposureAssetLast is greater than uint256(-deltaAsset) , exposureAssetLast is reduced by the value of deltaAsset and updated to lastExposuresAsset which would then be used for the next interaction and exposure, The problem is that when ever -deltaAsset is greater than exposureAssetLast, lastExposuresAsset is simply updated to zero, the excess value is not accounted for and would be simply lost to the contract, this excess value would have handled the subsequent exposure instead it is completely lost in the AbstractDerivedAM.sol contract

   // Struct with the exposures of a specific asset for a specific Creditor.
    struct ExposuresPerAsset {
        // The amount of exposure of the Creditor to the asset at the last interaction.
        uint112 lastExposureAsset;
        // The exposure in USD of the Creditor to the asset at the last interaction, 18 decimals precision.
        uint112 lastUsdExposureAsset;
    }

Impact

Denial of Service When the Excess Value of from DeltaAsset is enough to handle Subsequent Asset exposure

Code Snippet

https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/asset-modules/abstracts/AbstractDerivedAM.sol#L542 https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/asset-modules/abstracts/AbstractDerivedAM.sol#L49

Tool used

Manual Review

Recommendation

One way is for the Protocol to consider Using int instead of Uint for lastExposureAsset and other related variable to avoid completely lossing asset variable that could handle asset exposure during subsequent code execution in the AbstractDerivedAM.sol contract

Duplicate of #1

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[nevillehuang]

Invalid, agree with sponsors comments:

• Normally it can’t go under 0 in the first place, that would mean that one of the invariants is already broken
• We do set it to zero instead of reverting, so that assets are not stuck forever if this would happen.
• If it goes under 0 something is already pretty wrong, meaning that the asset module will probably set to allow withdrawals alone (maxExposure to 0) → no need to still do proper accounting going under 0