search

maple-labs / trail-of-bits-audit

This repo will be used for all communications between the Maple smart contracts team and the Trail of Bits auditing team.
0 stars 0 forks source link

Reentrancy in DebtLocker.setAuctioneer can lead to events being emitted out of order #4

Open snd opened 2 years ago

snd commented 2 years ago

This issue is very hard to produce and has a high difficulty.

Description

DebtLocker.setAuctioneer does not follow the checks-effects-interactions pattern.

https://github.com/maple-labs/debt-locker/blob/05b4f2fe119e2ddf3dc0e441055c602f748e7d52/contracts/DebtLocker.sol#L78-L83

The interaction Liquidator(_liquidator).setAuctioneer(auctioneer_); happens before the effect emit AuctioneerSet(auctioneer_);.

Exploit scenario

  1. DebtLocker.setAuctioneer(bob) is called
  2. Liquidator.setAuctioneer calls DebtLocker.setAuctioneer(alice) again (reentrancy)
  3. The event of the second (reentrant) call is emitted before the event of the first call
  4. The event of the first call is emitted
  5. To an off-chain event monitoring system it looks as if bob were the newest auctioneer. In reality alice is the newest auctioneer.

Again, this is an unlikely scenario, therefore this issue is low severity and high difficulty.

Recommendation

Short term, emit the event before the interaction.

Long term, integrate Slither into the CI pipeline. Slither can detect low severity reentrancies like this as well as high severity reentrancies.

deluca-mike commented 2 years ago

I imagine this issue would be littered all over our code, because we make the (fair) assumption of what the code in our other contracts are doing. But it's a good point, and something I'll consider when doing another once over of it all.

lucas-manuel commented 2 years ago

I like the idea about slither in CI, do you have any references for this?

snd commented 2 years ago

Slither is able to find all instances of this problem. We are going to add the other instances to this issue.

We'll get back to you on the question of integrating Slither into CI.

snd commented 2 years ago

One of our coworkers is currently working on a Github action that runs Slither. We'll let you know once that is finished.

lucas-manuel commented 2 years ago

Ok sounds good 👍

snd commented 2 years ago

Liquidator.liquidatePortion https://github.com/maple-labs/liquidations/blob/35c628e5ab45fbffaab7aef43a030a98b712a94a/contracts/Liquidator.sol#L41 suffers from a more severe version of this. ERC20Helper.transferFrom is reentrant if fundsAsset is an ERC777 token or similar token that is backwards compatible with ERC20 but allows callbacks for the sender and/or receiver.

snd commented 2 years ago

Similar for Liquidator.pullFunds https://github.com/maple-labs/liquidations/blob/35c628e5ab45fbffaab7aef43a030a98b712a94a/contracts/Liquidator.sol#L33.

snd commented 2 years ago

additional instances of this issue:

deluca-mike commented 2 years ago

Some of these are not really as important (as you note), since the order of many of these events is entirely irrelevant.

I'd agree that chronology is important for events that indicate the state of the contract, since, as you described above, an external viewer might be misled about the current auctioneer is, if the event's one job was to indicate that. However, for events indicating deltas PaymentMade, it's much less of an issue.

In anyway case, we will go over the contract to correct these issues, so long as the individual corrections for events that are deltas do not result in:

I imagine none will, but I do like to er on the side of efficiency rather than what some off-chain system needs.

lucas-manuel commented 2 years ago

I will make a checklist here to track progress (@deluca-mike @JGcarv you can check these boxes as well):

@snd please confirm that this includes all functions that should be examined

lucas-manuel commented 2 years ago

Made three tickets to track this issue, one for each repo where it applies:

snd commented 2 years ago

That checklist looks complete to me.

lucas-manuel commented 2 years ago

In MapleLoan, I believe that the following functions can remain as is due to the fact that the external calls are to ERC-20s and we can assume that the ERC-20s are whitelisted:

@snd is this reasonable?

snd commented 2 years ago

We'd recommend removing any reentrancies from those functions as well, by having them follow the checks-effects-interactions pattern. There might be some tokens that you would want to whitelist that can execute callbacks. Callbacks can be used to exploit reentrancy. ERC777 tokens for example, are backwards compatible with ERC20, so they look just like ERC20 tokens. However, they allow the sender and receiver to set up callbacks that are called on any transfer. This allows any sender and receiver to exploit reentrancies. In other words, reentrancies for tokens can not easily be avoided by whitelisting tokens. Unless you ensure that you only whitelist tokens that can not under any circumstances execute callbacks. This would be cumbersome, error prone and impossible to guarantee for upgradeable tokens.

snd commented 2 years ago

Being a security firm and erring on the side of security, we generally recommend adding reentrancy guards to all functions that do external calls. Doing so eliminates the entire class of reentrancy bugs and defaults to security. Defaulting to security is something we highly recommend for code intended to handle millions of dollars. Reentrancy guards can always be removed from individual functions that require additional optimization. Making the optimized, less-secure case the default is not something we recommend.

The only downside of reentrancy guards is their gas usage. OpenZeppelin's ReentrancyGuard for example requires one "cold" SLOAD (2100 gas cost), one "hot" "clean" SSTORE from non-zero to non-zero (2900 gas cost), and one "hot" "dirty" SSTORE changing the non-zero value back to the non-zero original (100 gas cost, 2800 gas refund). In total the gas cost is 2300. Assuming a "high" gas price of 200 gwei and current ether price of $4200, 2300 gas correspond to $1.8. We consider ~$2 a reasonable price to pay for the added security.

Reentrancy guards are very common among the protocols we audit. Not using reentrancy guards is unusual.

Using reentrancy guards also saves expensive developer time. The whole class of bugs can be addressed with one simple solution. Without reentrancy guards, each function that executes external calls has to be investigated and modified manually to remove reentrancies. This process must be repeated every time the function is modified. This introduces the likelihood of reentrancies being present in the code with potentially very costly consequences.

lucas-manuel commented 2 years ago

Made a PR in liquidations

lucas-manuel commented 2 years ago

Made a PR in debt-locker

lucas-manuel commented 2 years ago

Made a PR in maple-proxy-factory

snd commented 2 years ago

We reviewed the PRs and consider this Partially fixed