Skip to content
This repository has been archived by the owner on Mar 3, 2024. It is now read-only.

deth - Clearinghouse.sol#claimDefaulted() - Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function. #176

Open
sherlock-admin opened this issue Aug 28, 2023 · 5 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Aug 28, 2023

deth

high

Clearinghouse.sol#claimDefaulted() - Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function.

Summary

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.

@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Sep 1, 2023
@jkoppel
Copy link
Collaborator

jkoppel commented Sep 2, 2023

Seems real

ohmzeus added a commit to ohmzeus/Cooler that referenced this issue Sep 5, 2023
Summary: Clearinghouse.sol#claimDefaulted() - Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function.
Issue Link: sherlock-audit/2023-08-cooler-judging#176
Fix Description: Add OHM immutable ERC20 and approve MINTR before burning. Burn all OHM held in contract.
@0xrusowsky 0xrusowsky added Sponsor Confirmed The sponsor acknowledged this issue is valid Disagree With Severity The sponsor disputed the severity of this issue Will Fix The sponsor confirmed this issue will be fixed labels Sep 9, 2023
@0xrusowsky
Copy link

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
Copy link

0xrusowsky commented Sep 11, 2023

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)

@hrishibhat hrishibhat removed the Disagree With Severity The sponsor disputed the severity of this issue label Sep 12, 2023
@sherlock-admin2 sherlock-admin2 changed the title Immense Syrup Shetland - Clearinghouse.sol#claimDefaulted() - Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function. deth - Clearinghouse.sol#claimDefaulted() - Clearinghouse doesn't approve the MINTR to handle tokens in his name, which bricks the entire function. Sep 12, 2023
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Sep 12, 2023
@0xrusowsky
Copy link

@jkoppel
Copy link
Collaborator

jkoppel commented Sep 20, 2023

Fix approved.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants