Issue H-1: Can steal gOhm by calling Clearinghouse.claimDefaulted on loans not made by Clearinghouse

Source: #28

Found by

detectiveking, jkoppel, mert_eren

Clearinghouse.claimDefaulted assumes that all loans passed in were originated by the Clearinghouse. However, nothing guarantees that. An attacker can wreak havoc by calling it with a mixture of Clearinghouse-originated and external loans. In particular, they can inflate the computed totalCollateral recovered to steal excess gOhm from defaulted loans.

Vulnerability Detail

Alice creates a Cooler. 9 times, she calls requestLoan (not through the Clearinghouse) to request a loan of 0.000001 DAI collateralized by 2 gOhm. For each loan, she then calls clearLoan and loans the 0.000001 DAI to herself.
One week later, Bob calls Clearinghouse.lendToCooler and takes a loan for 3000 DAI collateralized by 1 gOHM
Alice defaults on the loans she made to herself and waits 7 days
Bob defaults on his loan
Alice calls Clearinghouse.claimDefaulted, passing in both her loans to herself and Bob's loan from the Clearinghouse. Clearinghouse.claimDefaulted calls Cooler.claimDefaulted on each, returning 18 gOhm to Alice and 1 gOhm to the Clearinghouse.
For each of Alice's loan, the keeper reward is incremented by the max award of 0.1 gOhm. For Bob's loan, the keeper reward is incremented by somewhere between 0 and 0.05 gOhm, depending on how much time has elapsed since Bob's loan defaulted.
The keeper reward is transferred to Alice. Alice's reward will be between 0.9 and 0.95 gOhm, but it should be between 0 and 0.05 gOhm. The contract should recover between 0.95 and 1 gOhm, but it only recovers between 0.05 and 0.1 gOhm. Alice has effectively stolen 0.9 gOhm from the contract

The attack as stated above can steal at most 5% of the collateral. Note that Alice can get this even without waiting 7 days from loan expiry time. It further requires the Clearinghouse have some extra gOhm around, as it will burn totalCollateral - keeperRewards. This can happen if the treasury or someone sends it some gOhm for some reason, or by calling claimDefault as in #3 .

However, #5 extends this attack so that Alice can steal 100% of the collateral, even if the Clearinghouse has no extra gOhm lying around.

For added fun, note that, when setting up her loans to herself, Alice can set the loan duration to 0 seconds. So this only requires setting up 1 block in advance.

Impact

Anyone can steal collateral from defaulted loans.

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Clearinghouse.sol#L191

Notice the lack of any checks that the loan's lender is the Clearingouse

    function claimDefaulted(address[] calldata coolers_, uint256[] calldata loans_) external {
        uint256 loans = loans_.length;
        if (loans != coolers_.length) revert LengthDiscrepancy();

        uint256 totalDebt;
        uint256 totalInterest;
        uint256 totalCollateral;
        uint256 keeperRewards;
        for (uint256 i=0; i < loans;) {
            // Validate that cooler was deployed by the trusted factory.
            if (!factory.created(coolers_[i])) revert OnlyFromFactory();
            
            // Claim defaults and update cached metrics.
            (uint256 debt, uint256 collateral, uint256 elapsed) = Cooler(coolers_[i]).claimDefaulted(loans_[i]);

keeperRewards is incremented for every loan.

            // Cap rewards to 5% of the collateral to avoid OHM holder's dillution.
            uint256 maxAuctionReward = collateral * 5e16 / 1e18;
            // Cap rewards to avoid exorbitant amounts.
            uint256 maxReward = (maxAuctionReward < MAX_REWARD)
                ? maxAuctionReward
                : MAX_REWARD;
            // Calculate rewards based on the elapsed time since default.
            keeperRewards = (elapsed < 7 days)
                ? keeperRewards + maxReward * elapsed / 7 days
                : keeperRewards + maxReward;
        }

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L318

Cooler.claimDefaulted can be called by anyone.

 function claimDefaulted(uint256 loanID_) external returns (uint256, uint256, uint256) {
        Loan memory loan = loans[loanID_];
        delete loans[loanID_];

       // Hey look, no checks on sender
}

Tool used

Manual Review

Recommendation

Check that the Clearinghouse is the originator of all loans passed to claimDefaulted

Discussion

0xRusowsky

ohmzeus/Cooler#48

jkoppel

Note on this:

The link to #3 is meant to be a link to #46

The link to #5 is meant to be a link to #115

In the past, when I linked to issues in my private judging repository, Sherlock would properly update them upon submission. Now it just links them to whatever issue in the public judging repo has the same number.

jkoppel

Fix confirmed.

Issue H-2: At claimDefaulted, the lender may not receive the token because the Unclaimed token is not processed

Source: #119

Found by

0xMAKEOUTHILL, Chinmay, Negin, banditx0x, deadrxsezzz, jkoppel, klaus, mahdikarimi, pengun, xAlismx

claimDefaulted does not handle loan.unclaimed . This preventing the lender from receiving the debt repayment.

Vulnerability Detail

function claimDefaulted(uint256 loanID_) external returns (uint256, uint256, uint256) {
  Loan memory loan = loans[loanID_];
  delete loans[loanID_];

Loan data is deletead in claimDefaulted function. loan.unclaimed is not checked before data deletead. So, if claimDefaulted is called while there are unclaimed tokens, the lender will not be able to get the unclaimed tokens.

Impact

Lender cannot get unclaimed token.

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/6d34cd12a2a15d2c92307d44782d6eae1474ab25/Cooler/src/Cooler.sol#L318-L320

Tool used

Manual Review

Recommendation

Process unclaimed tokens before deleting loan data.

function claimDefaulted(uint256 loanID_) external returns (uint256, uint256, uint256) {
+   claimRepaid(loanID_)
    Loan memory loan = loans[loanID_];
    delete loans[loanID_];

Discussion

0xRusowsky

fix: ohmzeus/Cooler#54
ohmzeus/Cooler#47

jkoppel

Fix approved.

Issue H-3: Clearinghouse.sol#claimDefaulted()

Source: #176

Found by

Ignite, deth Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function.

Vulnerability Detail

Inside claimDefaulted on the last line we call MINTR.burnOhm which in turn calls OHM.burnFrom. The docs for MINTR.burnFrom state: "Burn OHM from an address. Must have approval.". We can confirm that this is the case when looking at OHM source code and it's burnFrom. I found 2 OHM tokens that are currently deployed on mainnet, so I'm linking both their addresses: https://etherscan.io/token/0x383518188c0c6d7730d91b2c03a03c837814a899#code, https://etherscan.io/token/0x64aa3364f17a4d01c6f1751fd97c2bd3d7e7f1d5#code. Both addresses use the same burnFrom logic and in both cases they require an allowance. Nowhere in the contract do we approve the MINTR to handle OHM tokens in the name of Clearinghouse, in fact OHM isn't even specified in Clearinghouse.

Side note: The test testFuzz_claimDefaulted succeeds, because MockOhm is written incorrectly. When burnFrom gets called MockOhm calls the inherited _burn function, which burns tokens from msg.sender. The mock doesn't represent how the real OHM.burnFrom works.

Impact

Claimdefault will always revert.

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/6d34cd12a2a15d2c92307d44782d6eae1474ab25/Cooler/src/Clearinghouse.sol#L244

Tool used

Manual Review

Recommendation

Add a variable ohm which will be the OHM address and approve the necessary tokens to the MINTR before calling MINTR.burnOhm.

Discussion

jkoppel

Seems real

0xRusowsky

Confirmed, but disagree with the severity. Defaults could still happen via the Cooler contracts and OHM could be burned ad-hoc by the DAO.

0xRusowsky

After discussing it internally, we don't mind if it's labeled as high or medium cause we would need to deploy a new policy (so it would require some extra work on our end)

0xRusowsky

ohmzeus/Cooler#52

jkoppel

Fix approved.

Issue H-4: isCoolerCallback can be bypassed

Source: #187

Found by

0xbepresent, BugHunter101, Delvir0, Hama, Ignite, Kow, Mlome, Vagner, banditx0x, castle_chain, deadrxsezzz, detectiveking, evilakela, harisnabeel, jah, klaus, libratus, mert_eren, ni8mare, nmirchev8, ubermensch

The lender can bypass CoolerCallback.isCoolerCallback() validation without implements the CoolerCallback abstract.

In the provided example, this may force the loan to default.

Vulnerability Detail

The CoolerCallback.isCoolerCallback() is intended to ensure that the lender implements the CoolerCallback abstract at line 241 when the parameter isCallback_ is true.

function clearRequest(
    uint256 reqID_,
    bool repayDirect_,
    bool isCallback_
) external returns (uint256 loanID) {
    Request memory req = requests[reqID_];

    // If necessary, ensure lender implements the CoolerCallback abstract.
    if (isCallback_ && !CoolerCallback(msg.sender).isCoolerCallback()) revert NotCoolerCallback();

    // Ensure loan request is active. 
    if (!req.active) revert Deactivated();

    // Clear the loan request in memory.
    req.active = false;

    // Calculate and store loan terms.
    uint256 interest = interestFor(req.amount, req.interest, req.duration);
    uint256 collat = collateralFor(req.amount, req.loanToCollateral);
    uint256 expiration = block.timestamp + req.duration;
    loanID = loans.length;
    loans.push(
        Loan({
            request: req,
            amount: req.amount + interest,
            unclaimed: 0,
            collateral: collat,
            expiry: expiration,
            lender: msg.sender,
            repayDirect: repayDirect_,
            callback: isCallback_
        })
    );

    // Clear the loan request storage.
    requests[reqID_].active = false;

    // Transfer debt tokens to the owner of the request.
    debt().safeTransferFrom(msg.sender, owner(), req.amount);

    // Log the event.
    factory().newEvent(reqID_, CoolerFactory.Events.ClearRequest, 0);
}

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L233-L275

However, this function doesn't provide any protection. The lender can bypass this check without implementing the CoolerCallback abstract by calling the Cooler.clearRequest() function using a contract that implements the isCoolerCallback() function and returns a true value.

For example:

contract maliciousLender {
    function isCoolerCallback() pure returns(bool) {
        return true;
    }
    
    function operation(
        address _to,
        uint256 reqID_
    ) public {
        Cooler(_to).clearRequest(reqID_, true, true);
    }
    
    function onDefault(uint256 loanID_, uint256 debt, uint256 collateral) public {}
}

By being the loan.lender with implement only onDefault() function, this will cause the repayLoan() and rollLoan() methods to fail due to revert at onRepay() and onRoll() function. The borrower cannot repay and the loan will be defaulted.

After the loan default, the attacker can execute claimDefault() to claim the collateral.

Furthermore, there is another method that allows lenders to bypass the CoolerCallback.isCoolerCallback() function which is loan ownership transfer.

/// @notice Approve transfer of loan ownership rights to a new address.
/// @param  to_ address to be approved.
/// @param  loanID_ index of loan in loans[].
function approveTransfer(address to_, uint256 loanID_) external {
    if (msg.sender != loans[loanID_].lender) revert OnlyApproved();

    // Update transfer approvals.
    approvals[loanID_] = to_;
}

/// @notice Execute loan ownership transfer. Must be previously approved by the lender.
/// @param  loanID_ index of loan in loans[].
function transferOwnership(uint256 loanID_) external {
    if (msg.sender != approvals[loanID_]) revert OnlyApproved();

    // Update the load lender.
    loans[loanID_].lender = msg.sender;
    // Clear transfer approvals.
    approvals[loanID_] = address(0);
}

Normally, the lender who implements the CoolerCallback abstract may call the Cooler.clearRequest() with the _isCoolerCallback parameter set to true to execute logic when a loan is repaid, rolled, or defaulted.

But the lender needs to change the owner of the loan, so they call the approveTransfer() and transferOwnership() functions to the contract that doesn't implement the CoolerCallback abstract (or implement only onDefault() function to force the loan default), but the loan.callback flag is still set to true.

Thus, this breaks the business logic since the three callback functions don't need to be implemented when the isCoolerCallback() is set to true according to the dev note in the CoolerCallback abstract below:

/// @notice Allows for debt issuers to execute logic when a loan is repaid, rolled, or defaulted. /// @dev The three callback functions must be implemented if isCoolerCallback() is set to true.

Impact

The lender forced the Loan become default to get the collateral token, owner lost the collateral token.
Bypass the isCoolerCallback validation.

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L241

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L338-L343

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L347-L354

Tool used

Manual Review

Recommendation

Only allowing callbacks from the protocol-trusted address (eg., Clearinghouse contract).

Disable the transfer owner of the loan when the loan.callback is set to true.

Discussion

Oot2k

Duplicate of 30

Oot2k

Reorder issues

0xRusowsky

Issue M-1: `emergency_shutdown` role is not enough for emergency shutdown.

Source: #1

Found by

thekmj, ubermensch

There are two protocol roles, emergency_shutdown and cooler_overseer. The emergency_shutdown should have the ability to shutdown the Clearinghouse.

However, in the current contract, emergency_shutdown role does not have said ability. An address will need both emergency_shutdown and cooler_overseer to perform said action.

We have also confirmed with the protocol team that the two roles will be held by two different multisigs, with the shutdown multisig having a lower threshold and more holders. Thereby governance will not be able to act as quickly to emergencies than expected.

Vulnerability Detail

Let's examine the function emergencyShutdown():

function emergencyShutdown() external onlyRole("emergency_shutdown") {
    active = false;

    // If necessary, defund sDAI.
    uint256 sdaiBalance = sdai.balanceOf(address(this));
    if (sdaiBalance != 0) defund(sdai, sdaiBalance);

    // If necessary, defund DAI.
    uint256 daiBalance = dai.balanceOf(address(this));
    if (daiBalance != 0) defund(dai, daiBalance);

    emit Deactivated();
}

This has the modifier onlyRole("emergency_shutdown"). However, this also calls function defund(), which has the modifier onlyRole("cooler_overseer")

function defund(ERC20 token_, uint256 amount_) public onlyRole("cooler_overseer") {

Therefore, the role emergency_shutdown will not have the ability to shutdown the protocol, unless it also has the overseer role.

Proof of concept

To get a coded PoC, make the following modifications to the test case:

In Clearinghouse.t.sol, comment out line 125 (so that overseer only has emergency_shutdown role) https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/test/Clearinghouse.t.sol#L125

//rolesAdmin.grantRole("cooler_overseer", overseer);
rolesAdmin.grantRole("emergency_shutdown", overseer);

Run the following test command (to just run a single test test_emergencyShutdown()):

forge test --match-test test_emergencyShutdown

The test will fail with the ROLES_RequireRole() error.

Impact

emergency_shutdown role cannot emergency shutdown the protocol

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Clearinghouse.sol#L339 https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Clearinghouse.sol#L360-L372

Tool used

Manual Review, Foundry/Forge

Recommendation

There are two ways to mitigate this issue:

Separate the logic for emergency shutdown and defunding. i.e. do not defund when emergency shutdown, but rather defund separately after shutdown.
Move the defunding logic to a separate internal function, so that emergency shutdown function can directly call defunding without going through a modifier.

Discussion

sherlock-admin

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

0xyPhilic commented:

invalid because it can be considered low as roles can be given again and there is no loss of funds

0xRusowsky

fair point, but it still should be low as a user can have several roles

Oot2k

I have to disagree, a user can indeed have several roles, but that can not be ensured/ if there are two separate roles they should be considered separate.

ohmzeus

Fix: ohmzeus/Cooler#50

jkoppel

Fix confirmed.

Issue M-2: Lender is able to steal borrowers collateral by calling rollLoan with unfavourable terms on behalf of the borrower.

Source: #26

Found by

0xMosh, 0xbepresent, 0xmurali7, ADM, B353N, Breeje, BugHunter101, Chinmay, Delvir0, HChang26, Kow, Kral01, Mlome, SBSecurity, SanketKogekar, Silvermist, Yanev, banditx0x, carrotsmuggler, castle_chain, cats, deadrxsezzz, detectiveking, deth, evilakela, hals, jovi, libratus, mahdikarimi, ni8mare, nmirchev8, p-tsanev, pengun, sandy, tvdung94 A Lender is able to call provideNewTermsForRoll with whatever terms they want and then can call rollLoan on behalf of the borrower forcing them to roll the loan with the terms they provided. They can abuse this to make the loan so unfavourable for the borrower to repay that they must forfeit their collateral to the lender.

Vulnerability Detail

Say a user has 100 collateral tokens valued at $1,500 and they wish to borrow 1,000 debt tokens valued at $1,000 they would would call: (values have simplified for ease of math)

requestLoan("1,000 debt tokens", "5% interest", "10 loan tokens for each collateral", "1 year")

If a lender then clears the request the borrower would expect to have 1 year to payback 1,050 debt tokens to be able to receive their collateral back.

However a lender is able to call provideNewTermsForRoll with whatever terms they wish: i.e.

provideNewTermsForRoll("loanID", "10000000% interest", "1000 loan tokens for each collateral" , "1 year")

They can then follow this up with a call to rollLoan(loanID): During the rollLoan function the interest is recalculated using:

    function interestFor(uint256 amount_, uint256 rate_, uint256 duration_) public pure returns (uint256) {
        uint256 interest = (rate_ * duration_) / 365 days;
        return (amount_ * interest) / DECIMALS_INTEREST;
    }

As rate_ & duration_ are controllable by the borrower when they call provideNewTermsForRoll they can input a large number that the amount returned is much larger then the value of the collateral. i.e. input a rate_ of amount * 3 and duration of 365 days so that the interestFor returns 3,000.

This amount gets added to the existing loan.amount and would make it too costly to ever repay as the borrower would have to spend more then the collateral is worth to get it back. i.e. borrower now would now need to send 4,050 debt tokens to receive their $1,500 worth of collateral back instead of the expected 1050.

The extra amount should result in more collateral needing to be sent however it is calculated using loan.request.loanToCollateral which is also controlled by the lender when they call provideNewTermsForRoll, allowing them to input a value that will result in newCollateralFor returning 0 and no new collateral needing to be sent.

    function newCollateralFor(uint256 loanID_) public view returns (uint256) {
        Loan memory loan = loans[loanID_];
        // Accounts for all outstanding debt (borrowed amount + interest).
        uint256 neededCollateral = collateralFor(loan.amount, loan.request.loanToCollateral);  
        // Lender can force neededCollateral to always be less than loan.collateral

        return neededCollateral > loan.collateral ? neededCollateral - loan.collateral : 0;
    }

As a result a borrower who was expecting to have repay 1050 tokens to get back their collateral may now need to spend many multiples more of that and will just be forced to just forfeit their collateral to the lender.

Impact

Borrower will be forced to payback the loan at unfavourable terms or forfeit their collateral.

Code Snippet

Cooler.sol#L192-L217 Cooler.sol#L282-L300

Tool used

Manual Review

Recommendation

Add a check restricting rollLoan to only be callable by the owner. i.e.:

function rollLoan(uint256 loanID_) external {
        Loan memory loan = loans[loanID_];
        
        if (msg.sender != owner()) revert OnlyApproved();

Note: unrelated but rollLoan is also missing its event should add:

factory().newEvent(reqID_, CoolerFactory.Events.RollLoan, 0);

Discussion

jkoppel

Whether this is medium or high depends on how likely borrowers are to make massively over-collateralized loans

0xRusowsky

imo a Medium

Oot2k

escalate split frontrunning and access control into own issues

sherlock-admin2

escalate split frontrunning and access control into own issues

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0xRusowsky

Oot2k

Following issues are not duplicates of 26 and should be grouped together and treaded as another issue: 16 (#16) 18 (#18) 72 (#72) 99 (#99) 130 (#130) 137 (#137) 150 (#150) 204 (#204) 221 (#221) 243 (#243) 271 (#271)

226 -> Invalid

Oot2k

Addition: 226 shows attack path and root cause, mentions tokens that are not supported -> sherlock has to decide if valid/invalid 231 is not duplicate of this issue and should be grouped with the other ones mentioned above

hrishibhat

Result: Medium Has duplicates The respective set of issues has been separated

sherlock-admin2

Escalations have been resolved successfully!

Escalation status:

Oot2k: accepted

jkoppel

Fix confirmed. Sponsor agreed to accept some economic concerns with the fix, but no security concerns were identified.

Issue M-3: gOhm stuck forever if call claimDefaulted on Cooler directly

Source: #46

Found by

castle_chain, detectiveking, evilakela, jkoppel

Anyone can call Cooler.claimDefaulted. If this is done for a loan owned by the Clearinghouse, the gOhm is sent to the Clearinghouse, but there is no way to recover or burn it.

Vulnerability Detail

Bob calls Clearinghouse.lendToCooler to make a loan collateralized by 1000 gOhm.
Bob defaults on the loan
Immediately after default, Eve calls Cooler.claimDefaulted on Bob's loan.
The gOhm is transferred to the Clearinghouse
There is no way to burn or transfer it. (In fact, defund() can be used to transfer literally any token except gOhm back to the treasury.)

However, the gOhm can now be stolen using the exploit in #1, potentially in the same transaction as when Eve called Cooler.claimDefaulted().

Impact

Anyone can very easily make all defaulted gOhm get stuck forever.

Code Snippet

Cooler.claimDefaulted sends the collateral to the lender, calls onDefault

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L325

Clearinghouse.onDefault does nothing

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Clearinghouse.sol#L265

Although Clearinghouse.defund() can be used to send any other token back to the treasury, it cannot do so for gOhm

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Clearinghouse.sol#L340

Tool used

Manual Review

Recommendation

Unsure. Perhaps add a flag disabling claiming by anyone other than loan.lender? Or just allow defund() to be called on gOhm?

Discussion

jkoppel

This is not a duplicate of #28. #28 involves Clearinghouse.claimDefaulted, but this involves Cooler.claimDefaulted.

Oot2k

Not a duplicate

0xRusowsky

Despite it is not a duplicate, since gOHM would be stuck in CH instead of the being OHM burn. It wouldn't be a big deal (we could ammend the calculations based on that) because it doesn't have any operational/economical impact as long as that supply is removed from the backing calculations.

On top of that, there is an economical incentive to call it from the CH, as the caller is rewarded.

Disagree with severity, imo at max it should be a medium.

Will think about how to deal with it.

0xRusowsky

we will finally add a permissionless burn function despite this logic is unlikely to happen

0xRusowsky

ohmzeus/Cooler#57

jkoppel

Fix approved.

Issue M-4: Lender can front-run `rollLoan` and call `provideNewTermsForRoll` with unfavorable terms

Source: #243

Found by

0xbepresent, Breeje, banditx0x, cats, deadrxsezzz, detectiveking, evilakela, harisnabeel, james_wu, pep7siup, radevauditor, sandy, ubl4nk Lender can front-run rollLoan and result in borrower accepting unfavorable terms.

Vulnerability Detail

After a loan is created, the lender can provide new loan terms via provideNewTermsForRoll. If they are reasonable, the user can then accept them. However this opens up a risky scenario:

User A borrows from lender B
Lender B proposes new suitable terms
User A sees them and calls rollLoan to accept them
Lender B is waiting for this and sees the pending transaction in the mempool
Lender B front-runs user A's transaction and makes a new call to provideNewTermsForRoll will an extremely high interest rate
User A's transaction now executes and they've accepted unfavorable terms with extremely high interest rate

Impact

User may get mislead in to accepting unfavorable terms and overpaying interest

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L192 https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L282

Tool used

Manual Review

Recommendation

When calling rollLoan let the user pass a parameter consisting of the max interest rate they are willing to accept to prevent from such incidents.

Discussion

0xRusowsky

ohmzeus/Cooler#63

jkoppel

This is moot because rollLoan no longer exists.

sherlock-audit/2023-08-cooler-judging

Issue H-1: Can steal gOhm by calling Clearinghouse.claimDefaulted on loans not made by Clearinghouse

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue H-2: At claimDefaulted, the lender may not receive the token because the Unclaimed token is not processed

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue H-3: Clearinghouse.sol#claimDefaulted()

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue H-4: isCoolerCallback can be bypassed

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue M-1: emergency_shutdown role is not enough for emergency shutdown.

Found by

Vulnerability Detail

Proof of concept

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue M-2: Lender is able to steal borrowers collateral by calling rollLoan with unfavourable terms on behalf of the borrower.

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue M-3: gOhm stuck forever if call claimDefaulted on Cooler directly

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue M-4: Lender can front-run rollLoan and call provideNewTermsForRoll with unfavorable terms

Found by

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Discussion

Issue M-1: `emergency_shutdown` role is not enough for emergency shutdown.

Issue M-4: Lender can front-run `rollLoan` and call `provideNewTermsForRoll` with unfavorable terms