AbstractDerivedAM.sol#processIndirectDeposit() - Incorrect rounding direction will favor the user

Summary

Incorrect rounding direction will favor the user

Vulnerability Detail

When calculating the exposure of an asset when depositing, we use processIndirectDeposit

function processIndirectDeposit(
        address creditor,
        address asset,
        uint256 assetId,
        uint256 exposureUpperAssetToAsset,
        int256 deltaExposureUpperAssetToAsset
    ) public virtual override onlyRegistry returns (uint256 recursiveCalls, uint256 usdExposureUpperAssetToAsset) {
        bytes32 assetKey = _getKeyFromAsset(asset, assetId);

        // Calculate and update the new exposure to "Asset".
        uint256 exposureAsset = _getAndUpdateExposureAsset(creditor, assetKey, deltaExposureUpperAssetToAsset);

        (uint256 underlyingCalls, uint256 usdExposureAsset) = _processDeposit(exposureAsset, creditor, assetKey);

        if (exposureAsset == 0 || usdExposureAsset == 0) {
            usdExposureUpperAssetToAsset = 0;
        } else {
            // Calculate the USD value of the exposure of the upper asset to the underlying asset.
            usdExposureUpperAssetToAsset = usdExposureAsset.mulDivDown(exposureUpperAssetToAsset, exposureAsset);
        }

        unchecked {
            recursiveCalls = underlyingCalls + 1;
        }
    }

usdExposureUpperAssetToAsset is used in the calculation and mutation of lastUsdExposureProtocol

 function _processDeposit(uint256 exposureAsset, address creditor, bytes32 assetKey) //ok
        internal
        virtual
        returns (uint256 underlyingCalls, uint256 usdExposureAsset)
    {
    ...
(underlyingCalls_, usdExposureToUnderlyingAsset) = IRegistry(REGISTRY)
                    .getUsdValueExposureToUnderlyingAssetAfterDeposit(
                    creditor,
                    underlyingAsset,
                    underlyingId,
                    exposureAssetToUnderlyingAssets[i],
                    deltaExposureAssetToUnderlyingAsset
                );
                usdExposureAsset += usdExposureToUnderlyingAsset;
                unchecked {
                    underlyingCalls += underlyingCalls_;
                }
            }

            // Cache and update lastUsdExposureAsset.
            uint256 lastUsdExposureAsset = lastExposuresAsset[creditor][assetKey].lastUsdExposureAsset;
            // If usdExposureAsset is bigger than uint112, then check on usdExposureProtocol below will revert.
            lastExposuresAsset[creditor][assetKey].lastUsdExposureAsset = uint112(usdExposureAsset);

            // Cache lastUsdExposureProtocol.
            uint256 lastUsdExposureProtocol = riskParams[creditor].lastUsdExposureProtocol;

            // Update lastUsdExposureProtocol.
            unchecked {
                if (usdExposureAsset >= lastUsdExposureAsset) {
                    usdExposureProtocol = lastUsdExposureProtocol + (usdExposureAsset - lastUsdExposureAsset);
                } else if (lastUsdExposureProtocol > lastUsdExposureAsset - usdExposureAsset) {
                    usdExposureProtocol = lastUsdExposureProtocol - (lastUsdExposureAsset - usdExposureAsset);
                }
                // For the else case: (lastUsdExposureProtocol < lastUsdExposureAsset - usdExposureAsset),
                // usdExposureProtocol is set to 0, but usdExposureProtocol is already 0.
            }
            // The exposure must be strictly smaller than the maxExposure, not equal to or smaller than.
            // This is to ensure that all deposits revert when maxExposure is set to 0, also deposits with 0 amounts.
            if (usdExposureProtocol >= riskParams[creditor].maxUsdExposureProtocol) {
                revert AssetModule.ExposureNotInLimits();
            }
        }
        riskParams[creditor].lastUsdExposureProtocol = uint112(usdExposureProtocol);

usdExposureAsset is then added to lastUsdExposureProtocol and the value usdExposureProtocol is compared with maxUsdExposure.

The issue here is that when we calculate usdExposureUpperAssetToAsset we round down, after which we compare a value that contains usdExposureUpperAssetToAsset to maxUsdExposureProtocol which acts as an invariant, the idea is to disallow depositing of assets as to not over expose the protocol.

Because of the rounding direction, there will be less exposure recorded on a deposit, which favors the users of the protocol.

The protocol team state.

All rounding should be in the disadvantage of the user (eg. rounding down for assets, rounding up for liabilities).

Considering maxUsdExposureProtocol is a protocol value used in an invariant I consider this a Medium severity issue.

Note that processIndirectWithdrawal rounds down correctly, as when we withdraw we want to remove less exposure, thus favoring the protocol, while when depositing we should round up, as it will add more exposure, which favors the protocol.

Impact

Incorrect rounding will favor the users, instead of the protocol, which will allow for more exposure by the users.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use mulDivUp instead of mulDivDown

Duplicate of #1

sherlock-admin2 commented 9 months ago

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

takarez commented:

invalid

nevillehuang commented 9 months ago

Invalid, agree with sponsors comments:

Invalid, Not in favour user or protocol (risk manager can just set maxExposure to 1 less)

Unlike with conversions shares assets, this cannot be abused by looping

sherlock-audit / 2023-12-arcadia-judging

deth - AbstractDerivedAM.sol#processIndirectDeposit() - Incorrect rounding direction will favor the user #186