security notes

A Tale of Two Calls: How a Reentrancy Attack Can Take Over Maker's CDPs

adriro's photo
adriro
·

4 min read

Some time ago, I was reviewing an implementation of ERC-3156 and decided to hunt for some unprotected flashloan callbacks. While this depends on the specifics of each case, you usually need a check to restrict who is calling this hook, making it easy to quickly spot potential issues. Armed with codeslaw, I searched for "onFlashLoan" and started browsing the results. While everything looked more or less fine, one result caught my eye: the MultiplyProxyActions contract.

  function onFlashLoan(
    address initiator,
    address token,
    uint256 amount,
    uint256 fee,
    bytes calldata params
  ) public override returns (bytes32) {
    (
      uint8 mode,
      ExchangeData memory exchangeData,
      CdpData memory cdpData,
      AddressRegistry memory addressRegistry
    ) = abi.decode(params, (uint8, ExchangeData, CdpData, AddressRegistry));

    require(msg.sender == address(addressRegistry.lender), "mpa-untrusted-lender");

    ...

Note how the requirement is implemented using addressRegistry.lender, a parameter derived from user input. We can simply call this function and set this argument as ourselves to bypass the check. However, since the contract doesn’t hold any funds and the mechanics seemed a bit unusual, I decided to dig deeper to understand the bigger picture. It turns out this contract is part of the summer.fi suite and allows you to create leveraged CDPs in Maker. Essentially, it flashloans DAI, swaps it for collateral, deposits the collateral, and borrows DAI to repay the flashloan.

Now, why is the callback implemented like this? See, these operations are executed behind the well-known DSProxy contract—a basic smart account with a generic execute() endpoint that delegatecalls other contracts containing the logic. Everything starts with this wallet, which is the one delegatecalling into the MultiplyProxyActions contract. This setup seems straightforward at first, but when the implementation needs to request a flashloan, a key question comes up: which receiver should it set?

We cannot set the DSProxy contract as the receiver because it lacks the required interface. Remember, this wallet only provides a shallow endpoint to execute an arbitrary payload, and it will revert if it receives a call to onFlashLoan(...). The decision here is to set the MultiplyProxyActions contract itself as the receiver of the flashloan.

This creates an interesting scenario: the same contract is used with two (very) different types of calls. First, the wallet delegatecalls it to initiate the operation. Then, the same implementation contract is used as the receiver for the flashloan hook—this time called by the flashloan provider and, crucially, outside the original context. In the first instance, security is naturally provided by the delegatecall frame. However, in the second, it is not, as we are no longer within the proxy context the moment we call the flashloan provider.

So, how does the MultiplyProxyActions operate on the CDP during this callback? The implementation sets the MultiplyProxyActions contract as the owner of the CDP before initiating the flashloan and removes it once the operation is complete, effectively simulating a temporary allowance.

  function takeAFlashLoan(
    AddressRegistry memory addressRegistry,
    CdpData memory cdpData,
    bytes memory paramsData
  ) internal {
    IManager(addressRegistry.manager).cdpAllow(
      cdpData.cdpId,
      addressRegistry.multiplyProxyActions,
      1
    );

    IERC3156FlashLender(addressRegistry.lender).flashLoan(
      IERC3156FlashBorrower(addressRegistry.multiplyProxyActions),
      DAI,
      cdpData.requiredDebt,
      paramsData
    );

    IManager(addressRegistry.manager).cdpAllow(
      cdpData.cdpId,
      addressRegistry.multiplyProxyActions,
      0
    );
  }

During the callback frame, the implementation needs to swap the borrowed stables for collateral. To achieve this, it uses a generic interface that simply calls a target/calldata provided as arguments, likely populated with data from 1inch.

If the swap route contains an accidental or intentionally planted hook, it could enable a reentrancy into the MultiplyProxyActions contract, which currently has full control over the CDP. This means any malicious actor in between can also call the MultiplyProxyActions contract and freely manipulate the CDP, as the temporary allowance remains active during this window. Remember, this contract was meant to be delegatecalled from the DSProxy, so all its functions are publicly exposed without any access protection. This is fine as long as the context remains within the proxy, but once the temporary allowance kicks in things become unsafe.

Out of the many different ways this could go wrong, the best attack I could think of is to forward the ownership. There are a ton of technical details, but in short you can use the reentrancy in a clever way to add any arbitrary account as another owner of the CDP. This allows for a transparent take over that doesn't interfere with the leverage or deleverage actions of the ongoing operation. Once the transaction completes, the attacker can do pretty much anything: remove the original owner, effectively stealing the CDP, or let it sit dormant and speculate with a future attack once it becomes more attractive.

The issue was reported to the summer.fi team through their bug bounty program on Immunefi. Although deemed technically valid, the protocol considered it an intended design decision and decided no fix was required. Unfortunately, this also meant no rewards for me. I’ll continue hunting, hoping for better luck next time.

I'm an independent security researcher currently focused on security reviews of smart contracts. You can follow me on Twitter or check my portfolio.

EthereumSmart ContractsSecurity