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

Core interactions would still be accessible after protocol pause #71

Open
c4-bot-8 opened this issue Oct 17, 2024 · 0 comments
Open

Core interactions would still be accessible after protocol pause #71

c4-bot-8 opened this issue Oct 17, 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 sufficient quality report This report is of sufficient quality

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L198
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L240-L304

Vulnerability details

Proof of Concept

By design it is expected that when the CDPVault.sol contract is paused by the Dao, core functionalities like the deposits and withdrawals should no longer be accessible.

Which is one of the reasons why the pauser role is granted to begin with in the constructor: https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L198

        _grantRole(PAUSER_ROLE, config.pauseAdmin);

Issue however is that the core functionalities like deposit all lack the whenNotPaused modifier which then means that even if the protocol gets paused, users can still integrate the vault, see https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/CDPVault.sol#L240-L304

    function deposit(address to, uint256 amount) external returns (uint256 tokenAmount) {
        tokenAmount = wdiv(amount, tokenScale);
        int256 deltaCollateral = toInt256(tokenAmount);
        modifyCollateralAndDebt({
            owner: to,
            collateralizer: msg.sender,
            creditor: msg.sender,
            deltaCollateral: deltaCollateral,
            deltaDebt: 0
        });
    }

    function withdraw(address to, uint256 amount) external returns (uint256 tokenAmount) {
        tokenAmount = wdiv(amount, tokenScale);
        int256 deltaCollateral = -toInt256(tokenAmount);
        modifyCollateralAndDebt({
            owner: to,
            collateralizer: msg.sender,
            creditor: msg.sender,
            deltaCollateral: deltaCollateral,
            deltaDebt: 0
        });
    }

    function borrow(address borrower, address position, uint256 amount) external returns (int256 deltaDebt) {
        uint256 scaledAmount = wdiv(amount, poolUnderlyingScale);
        deltaDebt = toInt256(scaledAmount);
        modifyCollateralAndDebt({
            owner: position,
            collateralizer: position,
            creditor: borrower,
            deltaCollateral: 0,
            deltaDebt: deltaDebt
        });

        return deltaDebt;
    }

    function repay(address borrower, address position, uint256 amount) external returns (int256 deltaDebt){
        uint256 scaledAmount = wdiv(amount, poolUnderlyingScale);
        deltaDebt = -toInt256(scaledAmount);
        modifyCollateralAndDebt({
            owner: position,
            collateralizer: position,
            creditor: borrower,
            deltaCollateral: 0,
            deltaDebt: deltaDebt
        });

        return deltaDebt;
    }

Also this seems to be a change to sort this issue out from the previous scope, however instead of implementing the suggested changes of applying the pausable modifier to the internal function being called it's not being done and instead no protection can be provided once the protocol is paused.

Impact

Pausing functionality is broken, users can still deposit. withdraw, borrow, repay even if protocol is paused due to say a blackswan event or whatsoever.

Recommended Mitigation Steps

Attach the whenNotPaused modifier to all 4 of deposit. withdraw, borrow, repay.

Assessed type

Context

@c4-bot-8 c4-bot-8 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 17, 2024
c4-bot-9 added a commit that referenced this issue Oct 17, 2024
@c4-bot-12 c4-bot-12 added the 🤖_primary AI based primary recommendation label 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 sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants