Execution is transferred to untrusted address factories from privileged context

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L381 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L197-L198

Vulnerability details

Impact

Both NFTCollectionFactory.createNFTDropCollectionWithPaymentFactory and NFTCollection.mintWithCreatorPaymentFactory allow the caller to pass an “address factory” target address and calldata that will be called via AddressLibrary.callAndReturnContractAddress in order to dynamically retrieve a recipient address. AddressLibrary checks that the return value of this call ABI-decodes to an address and that the returned address is a contract.

These are juicy targets for exploits, since they allow the caller to force NFTCollectionFactory and NFTCollection to call an arbitrary address with arbitrary calldata in a privileged context. (For example, NFTCollectionFactory is the owner of the collection and drop implementation templates).

We spent a lot of time trying to use these to destroy the template contracts and do other malicious things. (Alas, with no success). However, it's possible for contracts to be upgraded in the future in a way that makes an attack possible. Although there may not be a feasible exploit now, the potential impact is severe enough that we think it's worth noting.

Proof of Concept

It’s almost possible to self destruct the template implementations with an address factory that calls NFTDropCollection.selfDestruct() (which then reverts because of a lack of return data that can be ABI-decoded as an address).

Using this as the paymentAddressFactoryCall:

    CallWithoutValue memory selfDestructCall = CallWithoutValue({
      target: address(nftDropCollectionTemplate),
      callData: abi.encodePacked(NFTDropCollection.selfDestruct.selector)
    });

We get the following stack trace:

Running 1 test for test/foundry/KarmaBoom.sol:KarmaBoom
[FAIL. Reason: EvmError: Revert] testSelfDestruct() (gas: 24307)
Traces:
  [24307] KarmaBoom::testSelfDestruct() 
    ├─ [16319] NFTCollectionFactory::createNFTDropCollectionWithPaymentFactory("name", "symbol", "baseURI", 0x0000000000000000000000000000000000000000000000000000000000000000, 8888, 0x0000000000000000000000000000000000000000, 42, (0xefc56627233b02ea95bae7e19f648d7dcd5bb132, 0x9cb8a26a)) 
    │   ├─ [10783] NFTDropCollection::selfDestruct() 
    │   │   ├─ emit SelfDestruct(admin: NFTCollectionFactory: [0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    │   │   └─ ← ()
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Similarly, it's possible to re-enter the factory contract:

function testCreateNFTDropCollectionWithPaymentFactoryReentrancy() public {
    nftCollectionFactory.createNFTDropCollectionWithPaymentFactory(
      "name",
      "symbol",
      "baseURI",
      0,
      8888,
      address(0),
      42,
      // does not work because it does not return an address
      CallWithoutValue({
        target: address(nftCollectionFactory),
        callData: abi.encodePacked(
          NFTCollectionFactory.createNFTDropCollectionWithPaymentFactory.selector,
          abi.encode(
            "reentrancy-name",
            "reentrancy-symbol",
            "reentrancy-baseURI",
            0,
            8888,
            address(0),
            42,
            CallWithoutValue({
              target: address(nftDropCollectionTemplate),
              callData: abi.encodePacked(bytes4(keccak256("owner()")))
            })
          )
        )
      })
    );
  }

Since neither of these calls return an address, both attacks revert.

However, since the Foundation contracts are upgradeable, it would be possible to introduce a function in the future that is exploitable by an AddressLibrary call.

Recommended Mitigation Steps

Consider whether you can constrain this pattern further to only allow calls from address factories to a set of known good addresses.

Consider removing support for self-destruction in NFTDropCollection and NFTCollection, if it is successfully pulled off by an attacker it will be devastating to the protocol, creators and buyers.

Consider adding a reentrancy guard to functions that hand off execution to external contracts NFTCollectionFactory.createNFTDropCollectionWithPaymentFactory and NFTCollection.mintWithCreatorPaymentFactory

[HardlyDifficult]

ACK.

We believe this is Low risk. Ultimately the warden notes that no real attacks have been identified yet.

It is correct that the factory and NFT collection have ways of calling arbitrary contracts. However as the warden had noted, it must result in an address returned otherwise the call will revert -- this is why the WIP POC here reverts.

It's worth noting that neither of these contracts are intended to hold any assets nor should they be given any special permissions with other contracts - so there should not be anything at risk.

We understand that the pattern here may be considered risky. We believe there are no real attacks available to worry about here - but will promptly address anything that comes up in C4 contests, our immunify bounty program, or seen in the wild. We have used a similar pattern on mainnet for many months without any issues so far.

The approach used here is to remain very flexible to support other use cases. Our app uses this in order to create PercentSplitETH contracts -- however users may prefer other payment manager contracts such as 0x splits and we wanted our NFT and factory contracts to remain credibly neutral. We aim to never assume using one contract we have created assumes that it will only interact with other contracts made by us. When multiple contracts are involved, we allow users to pick and choose which they piece together

[HickupHH3]

Because there isn't concrete proof of an attack path, the medium severity rating is unjustified. Will consider it to be a low risk issue until proven otherwise.

Part of warden's QA: #208