ERC777 reentrancy when withdrawing can be used to withdraw all collateral

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Market.sol#L464

Vulnerability details

Impact

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.

Proof Of Concept

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.

Recommended Mitigation Steps

Mark these functions as nonReentrant.

[0xean]

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.

[c4-sponsor]

08xmt marked the issue as disagree with severity

[c4-sponsor]

08xmt marked the issue as sponsor acknowledged

[08xmt]

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.

[0xean]

@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.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[08xmt]

@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.

[0xean]

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.

[c4-judge]

0xean marked the issue as selected for report

[aasharck]

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.