Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modifyCollateralAndDebt() doesn't work as intended when the vault is paused since debts can still be increased #32

Open
c4-bot-6 opened this issue Oct 15, 2024 · 0 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation 🤖_16_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L422-L533

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L422-L533

    function modifyCollateralAndDebt(
        address owner,
        address collateralizer,
        address creditor,
        int256 deltaCollateral,
        int256 deltaDebt
    ) public {
        if (
            // position is either more safe than before or msg.sender has the permission from the owner
            ((deltaDebt > 0 || deltaCollateral < 0) && !hasPermission(owner, msg.sender)) ||
            // msg.sender has the permission of the collateralizer to collateralize the position using their cash
            (deltaCollateral > 0 && !hasPermission(collateralizer, msg.sender)) ||
            // msg.sender has the permission of the creditor to use their credit to repay the debt
            (deltaDebt < 0 && !hasPermission(creditor, msg.sender))
        ) revert CDPVault__modifyCollateralAndDebt_noPermission();

        // if the vault is paused allow only debt decreases
        if (deltaDebt > 0 || deltaCollateral != 0){
            _requireNotPaused();//@audit
        }

        Position memory position = positions[owner];
        DebtData memory debtData = _calcDebt(position);

        uint256 newDebt;
        uint256 newCumulativeIndex;

        uint256 profit;
        int256 quotaRevenueChange;
        ..snip

        emit ModifyCollateralAndDebt(owner, collateralizer, creditor, deltaCollateral, deltaDebt);
    }

This function is used to modify a Position's collateral and debt balances, it checks that the global debt ceiling and the vault's debt ceiling have not been exceeded via the CDM, etc.

Now one of the new changes in scope as hinted by the @audit tag is that in the case where the vault is paused only debt decreases should be accepted, however this is not enforced, considering a call is made to _requireNotPaused() that lacks any implementation across scope whatsoever, thereby allowing for debt to be increased even if the vault is paused.

Impact

Modifying the position is broken, considering even if the revert that's supposed to occur here during an increase of debt whenever protocol is paused would not, since _requireNotPaused() lacks any implementation whatsoever.

Recommended Mitigation Steps

Implement the _requireNotPaused() functionality and correctly check that the protocol is indeed not paused in the case where an attempt is made to increase the debt.

Assessed type

Context

@c4-bot-6 c4-bot-6 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 15, 2024
c4-bot-10 added a commit that referenced this issue Oct 15, 2024
@c4-bot-12 c4-bot-12 added 🤖_16_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 18, 2024
howlbot-integration bot added a commit that referenced this issue Oct 19, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation 🤖_16_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants