A `delegatecall` to `createBasket` compromises `Auction` and `Basket` references to `Factory` contract.

[code423n4]

Handle

broccolirob

Vulnerability details

A malicious "publisher" can deploy a manipulated spoof of Factory.sol that includes a delegatecall to the real Factory.sol contract for invoking createBasket().

https://github.com/code-423n4/2021-12-defiprotocol/blob/main/contracts/contracts/Factory.sol#L97-L120

When newAuction.initialize(address(newBasket), address(this)); is run, the spoofed version's address will be passed to the newly cloned Auction contract instead of the real one. Also, when initialize() is called on the newly cloned Basket contract, msg.sender will be equal to the spoofed contract, so that factory will be set incorrectly as well.

Impact

The Auction contract uses its factory reference for important calculations in bondForRebalance() and settleAuction(). https://github.com/code-423n4/2021-12-defiprotocol/blob/main/contracts/contracts/Auction.sol#L65-L67

• factory.bondPercentDiv() can return 0 so that bonds are free.

https://github.com/code-423n4/2021-12-defiprotocol/blob/main/contracts/contracts/Auction.sol#L97-L111

• factory.auctionMultiplier() and factory.auctionDecrement() can be manipulated to effect the ibRatio variable in the basket, setting it very high. This would allow the malicious "publisher" to burn a small number of basket tokens in exchange for all underlying tokens, thus draining the actual tokens provided by other basket holders.

https://github.com/code-423n4/2021-12-defiprotocol/blob/main/contracts/contracts/Basket.sol#L144-L145

• handleFees() in the Basket contract can be manipulated to return 0 for the ownerSplit() or, simply return the malicious "publisher" address as the owner.

Since view functions have access to msg.sender, the spoof contract can return certain values selectively. It might function normally for all typical users, giving the appearance everything is fine, but then return incorrect values when msg.sender == publisher.

Proof of Concept

1. Mallory creates standard basket proposal by calling proposeBasketLicense() on real Factory.
2. Mallory deploys MalloryFactory.sol that conforms to the IFactory interface and mirrors the real Factory.sol contract's methods and state values.
3. Mallory calls special attack() method that delegates a call to the real factory. address(realFactory).delegatecall(abi.encodeWithSignature("createBasket(uint256)", proposalId));.
4. Normal users mint tokens on that basket, not knowing a shadow factory has been substituted for the real one.
5. Mallory submits a new index following proper publishNewIndex() flow.
6. Mallory calls bondForRebalance().
7. After 24 hour wait, Mallory executes the following attackin one transaction:
   • Takes out a flash loan for ERC20 pendingTokens.
   • Uses transfer to send them directly to the basket contract.
   • Calls settleAuction().
   • factory.auctionMultiplier() is called on shadow factory and it see's msg.sender == address(mallory) so it returns a large number. factory.auctionDecrement() is called on shadow factory and returns 0.
   • basket.updateIBRatio() sets basket's ratio to value, much higher than it should be.
   • Mallory calls burn() to drain underlying tokens.

Tools Used

Manual review and Hardhat.

Recommended Mitigation Steps

Create an immutable variable that is set to the Factory contract's address when it gets deployed. Then, inline or in a modifier, check to make sure that address(this) == originalAddress.

[frank-beard]

This issue seems similar to a project forking another project and rugging with different code. Users should confirm the basket they are interacting with is a legitimate basket created from the correct factory, similar to using a uniswap pool or router contract.

[0xleastwood]

Agree with the sponsor, there is no reason why users would interact with a malicious fork of these contracts.