Platform: Code4rena
Start Date: 25/10/2022
Pot Size: $50,000 USDC
Total HM: 18
Participants: 127
Period: 5 days
Judge: 0xean
Total Solo HM: 9
Id: 175
League: ETH
Rank: 6/127
Findings: 4
Award: $3,456.08
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: Lambda
3397.8465 USDC - $3,397.85
Markets can be deployed with arbitrary tokens for the collateral, including ERC777 tokens (that are downwards-compatible with ERC20). However, when the system is used with those tokens, an attacker can drain his escrow contract completely while still having a loan. This happens because with ERC777 tokens, there is a tokensToSend hook that is executed before the actual transfer (and the balance updates) happen. Therefore, escrow.balance() (which retrieves the token balance) will still report the old balance when an attacker reenters from this hook.
We assume that collateral is an ERC777 token and that the collateralFactorBps is 5,000 (50%). The user has deposited 10,000 USD (worth of collateral) and taken out a loan worth 2,500 USD. He is therefore allowed to withdraw 5,000 USD (worth of collateral). However, he can usse the ERC777 reentrancy to take out all 10,000 USD (worth of collateral) and still keep the loaned 2,500 USD:
1.) The user calls withdraw(amount) to withdraw his 5,000 USD (worth of collateral).
2.) In withdrawInternal, the limit check succeeds (the user is allowed to withdraw 5,000 USD) and escrow.pay(to, amount) is called. This will initiate a transfer to the provided address (no matter which escrow is used, but we assume SimpleERC20Escrow for this example).
3.) Because the collateral is an ERC777 token, the tokensToSend hook is executed before the actual transfer (and before any balance updates are made). The user can exploit this by calling withdraw(amount) again within the hook.
4.) withdrawInternal will call getWithdrawalLimitInternal, which calls escrow.balance(). This receives the collateral balance of the escrow, which is not yet updated. Because of that, the balance is still 10,000 USD (worth of collateral) and the calculated withdraw limit is therefore still 5,000 USD.
5.) Both transfers (the reentered one and the original one) succeed and the user has received all of his collateral (10,000 USD), while still having the 2,500 USD loan.
Mark these functions as nonReentrant.
#0 - 0xean
2022-11-05T15:24:44Z
Sponsor should review as the attack does seem valid with some pre-conditions (ERC777 tokens being used for collateral). Probably more of a M severity.
#1 - c4-sponsor
2022-11-14T09:17:42Z
08xmt marked the issue as disagree with severity
#2 - c4-sponsor
2022-11-14T09:17:51Z
08xmt marked the issue as sponsor acknowledged
#3 - 08xmt
2022-11-14T09:18:43Z
We make the security assumption that future collateral added by Inverse Finance DAO is compliant with standard ERC-20 behavior. Inverse Finance is full control of collateral that will be added to the platform and only intend to add collateral that properly reverts on failed transfers. Each ERC20 token added as collateral will be audited for non-standard behaviour. I would consider this a Low Risk finding, depending on how you value errors made in launch parameters.
#4 - 0xean
2022-11-28T17:19:38Z
@08xmt - the revert on a failed transfer here isn't the issue, it is the re-entrancy that isn't guarded against properly. While I understand your comment, If it were my codebase, I would simply add the modifier and incur the small gas costs as an additional layer of security to avoid mistakes in the future. I don't think this is qualifies as H but does show an attack path that could be achieved with an ERC777 token being used as collateral. Going to downgrade to M and will be happy to hear more discussion on the topic before final review.
#5 - c4-judge
2022-11-28T17:19:43Z
0xean changed the severity to 2 (Med Risk)
#6 - c4-judge
2022-11-28T19:35:31Z
0xean marked the issue as satisfactory
#7 - 08xmt
2022-11-30T18:02:47Z
@0xean The risk is still only present with unvetted contracts, and if the desire should exist in the future to implement a market with a token with re-entrancy, the code can be modified as necessary.
Will respect the judge's decision on severity in the end, but ultimately seem like a deployment parameter risk more than anything.
#8 - 0xean
2022-12-01T15:46:07Z
thanks @08xmt for the response.
While I agree that proper vetting could avoid this issue, the wardens are analyzing the code and documentation that is presented before them and I think in light of this, the issue is valid. Had the warden simply stated that there was a reentrancy modifier missing without showing a valid path to it being exploited, I would downgrade to QA. But given they showed a valid attack path due to the lack of reentrancy controls I think this should be awarded.
#9 - c4-judge
2022-12-01T16:02:29Z
0xean marked the issue as selected for report
#10 - aasharck
2023-01-25T14:39:57Z
I don't think this was a problem at all. By the way, I am a noob, so forgive me if I am wrong.
I was looking at ERC777 vulnerability and found this.
And, I tried replicating almost the same thing with the same attack steps pointed out by the warden but it actually did not work.
So, here even if the attacker uses a contract and registers it with ERC777TokensSender (using ERC1820), the tokenToSend hook won't be called after the escrow.pay() function because the tokens are being sent from the escrow contract to the attacker contract.
tokensToSend will only be triggered whenever a registered holder’s (from) tokens are about to be moved or destroyed (docs: https://docs.openzeppelin.com/contracts/2.x/api/token/erc777#IERC777Sender). So this means that triggering only happens when we deposit the collateral and not when escrow.pay() executes
The only way the escrow.pay() can trigger the hook is by registering the contract with the ERC777TokensRecipient interface. But that means the balances will be updated and the attack won't work.
Again forgive me if I am wrong. I was confused and spent a lot of time testing this.
🌟 Selected for report: trustindistrust
Also found by: 0xbepresent, Jujic, Lambda, RaoulSchaffranek, c7e7eff, catchup, codexploder, cryptonue, d3e4, eierina, jwood, pashov, peanuts, pedroais, simon135
33.634 USDC - $33.63
https://github.com/code-423n4/2022-10-inverse/blob/aaf717f13c1f9a6e2f0888fef7c9b40400eeb4ec/src/DBR.sol#L81 https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Oracle.sol#L61
An operator in the Inverse protocol has a lot of power and can cause the protocol to be completely drained:
setFixedPrice, the collateral value can be arbitrarily controlled. If a very high value is set there, users will be able to take out huge loans, causing DOLA to lose its peg.DBR.addMinter. When a malicious minter is added there, loans can be taken out without paying interest (simply by minting more DBR). Furthermore, such a malicious action would probably destroy the value of DBR because users lose trust in the system.Note that these kind of issues have been marked as medium severity historically, so I am also using this severity here.
Consider implementing a governance system for these actions.
#0 - c4-judge
2022-11-05T21:06:06Z
0xean marked the issue as primary issue
#1 - c4-sponsor
2022-11-08T12:09:30Z
08xmt marked the issue as sponsor disputed
#2 - 08xmt
2022-11-08T12:10:01Z
The operator should always be Inverse Finance Governance, which is a timelocked governance contract.
#3 - c4-judge
2022-11-28T19:35:31Z
0xean marked the issue as satisfactory
#4 - 0xean
2022-11-28T23:40:23Z
@08xmt - I understand that governance is in place here an that it also has a time locked functionality, unfortunately that wasn't highlighted in any of the READMEs and wardens have submitted several issues based on the work they have done based on the information provided upfront.
Historically, C4 has errored on batching a lot of these types of issues into duplicates that highlight all the ways in which a contract can be controlled by an admin.
There has been a lot of recent discussion on the topic, but no consensus has been established. (https://github.com/code-423n4/org/issues/59)
For that reason, I am going to batch all of these issues into duplicates and award as M. I think the C4 org would welcome your input as a sponsor if you are willing to comment in that thread.
#5 - 08xmt
2022-11-30T11:06:52Z
@0xean That's fair. Can understand that from a warden PoV there was not enough info in the repo, and it was better to err on the side of caution and highlight issues that an EOA operator would have for the security of the protocol. Can change status to acknowledged, if desired.
#6 - c4-judge
2022-12-07T08:22:05Z
Simon-Busch marked the issue as duplicate of #301
🌟 Selected for report: adriro
Also found by: 8olidity, BClabs, CertoraInc, Chom, Franfran, Lambda, RaoulSchaffranek, Ruhum, codexploder, cryptphi, eierina, joestakey, kaden, neumo, pashov, rvierdiiev, sorrynotsorry
24.2165 USDC - $24.22
Some tokens have more than 18 decimals (e.g., YAM-V2 with 24 decimals) and a Chainlink feed can have 18 decimals. The current oracle contract will not work with this combination. We will have:
uint8 decimals = 36 - feedDecimals - tokenDecimals = 36 - 18 - 24
Which underflows and reverts.
In practice, all Chainlink USD feeds have 8 decimals, so if those are used, the issue cannot arise. However, it looks to me like those are not the only one that are intended to be used. Otherwise, a simple require(feeds[token].feed.decimals() == 8); could be added and it would not be necessary to have all this logic for varying feed decimals. Furthermore, the used mock oracle has 18 decimals, which also indicates that those are supported.
So let's say a YAM-V2/DOLA oracle with 18 decimals is used. This can be for instance a custom oracle (because no such direct oracle exists) that first calculates YAM-V2/ETH and then ETH/USD. When this oracle is used with Inverse, all calls to getPrice or viewPrice will revert and the market is completely unusable.
Incorporate the possibility of tokens with >18 decimals into the normalization.
#0 - c4-judge
2022-11-05T20:48:29Z
0xean marked the issue as duplicate
#1 - c4-judge
2022-11-28T16:07:00Z
0xean marked the issue as not a duplicate
#2 - c4-judge
2022-11-28T16:07:08Z
0xean marked the issue as duplicate of #540
#3 - Simon-Busch
2022-12-05T15:33:21Z
Issue marked as satisfactory as requested by 0xean
#4 - c4-judge
2022-12-07T08:18:20Z
Simon-Busch marked the issue as duplicate of #533
🌟 Selected for report: rbserver
Also found by: 0x1f8b, 0xNazgul, 0xc0ffEE, 8olidity, Aymen0909, Chom, Franfran, Jeiwan, Jujic, Lambda, M4TZ1P, Olivierdem, Rolezn, Ruhum, TomJ, Wawrdog, __141345__, bin2chen, c7e7eff, carlitox477, catchup, cccz, codexploder, cuteboiz, d3e4, dipp, djxploit, eierina, elprofesor, hansfriese, horsefacts, idkwhatimdoing, imare, immeas, joestakey, ladboy233, leosathya, martin, minhtrng, pashov, peanuts, pedroais, rokinot, rvierdiiev, saneryee, sorrynotsorry, tonisives
0.385 USDC - $0.38
Oracle.viewPrice (and Oracle.getPrice) use the result of latestAnswer() directly. This function is deprecated (see for instance the comment on Etherscan) and it is strongly recommended to use latestRoundData() instead. The problem with the current usage in Inverse is that the age of the answer is not checked. The answer can be arbitrarily old, but it will still be set as the price of the current day.
There are multiple scenarios where the current usage leads to undesired situations:
1.) Let's say the price of an asset decreased signficantly (e.g., 50%) over multiple days, but the chainlink feed was not updated for some reason. Inverse will still use the old price, which opens up an arbitrage possibility (buying on an exchange cheaply, taking out a loan and never paying it back) that could ultimately kill the protocol.
2.) We assume that the turnover of a day (e.g., the point in time where day is increased) happens at timestamp $t$. When the price fluctuates heavily around this time, this can lead to wrong behavior. Let's say a price was x at timestamp $t - 300$ and 1.1x at timestamp $t + 300$. In theory, the lowest recorded price for the most recent day should be 1.1x (that is the only price that was recorded on that day), but it will be x when getPrice is called between timestamps $t$ and $t + 299$.
Use latestRoundData() which provides the timestamp. This timestamp can then be used to calculate the day (instead of the current block timestamp).
#0 - neumoxx
2022-10-31T08:50:15Z
Duplicate of #601
#1 - c4-judge
2022-11-05T17:50:50Z
0xean marked the issue as duplicate
#2 - Simon-Busch
2022-12-05T15:25:51Z
Issue marked as satisfactory as requested by 0xean
#3 - c4-judge
2022-12-07T08:14:15Z
Simon-Busch marked the issue as duplicate of #584