Foundation Drop contest - 0xmatt's results

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#L381 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/AddressLibrary.sol#L29

Vulnerability details

Impact

The Ethereum Virtual Machine doesn't support concurrency. When calling an external address, control flow is transferred to the external entity. If a called contract is acting in bad faith that could have implications for the calling contract or project. For example, an attacker could invoke functions from a tainted (e.g. blacklisted) contract, potentially leading to the contract's and associated addresses being tainted. Or alternatively the contract could be used to attack another contract.

The call could also be used to re-enter the contract, although I haven't found anything significant that can be changed by re-entering in this instance. I have created a separate issue for all re-entrancy issues found.

Having reviewed the judging criteria and spoken to @HardlyDifficult I submitted this finding as Medium. This could be abused to interact with a blacklistable contract (e.g. Tornado Cash), which in turn could result in blacklist contagion. That's too hand-wavy to be a high for me, but still could affect users.

I also recognize (thanks to @HardlyDifficult) that they have reasons for their choices and were very open in discussion. I'd like to thank them for their openness and assistance.

Proof of Concept

There are 4 elements to this issue:

1. Anyone (not just users) can use this. The number of potential abusers is anyone on-chain.
2. The user-control of the destination contract address.
3. The user-control of the destination calldata.
4. The call permits state changes in any contract on-chain.

The function createNFTDropCollectionWithPaymentFactory() has a user-supplied argument paymentAddressFactoryCall. This is passed to a call to the _createNFTDropCollection() function via a call to AddressLibrary.callAndReturnContractAddress(paymentAddressFactoryCall).

This then calls AddressLibrary.callAndReturnContractAddress(), which uses OpenZeppelin's AddressUpgradeable.sol#functionCall().

This then jumps through several functions before resolving to functionCallWithValue() in OpenZeppelin's AddressUpgradeable.sol. The relevant call in AddressUpgradeable.sol is on Line 137:

(bool success, bytes memory returndata) = target.call{value: value}(data);

Although this is a .call and not a .delegateCall the values of target and data are both under attacker control.

Tools Used

VSCode

Recommended Mitigation Steps

Mitigation requires addressing one or more of the issue's 3 elements:

1. Reducing the number of potential abusers will partially mitigate risk.
2. Removing the contract address from the call may mitigate risk.
3. Removing the destination calldata from user control may partially mitigate risk as an attacker would still control execution flow.
4. Making the call a staticcall would still allow the contract to dynamically resolve payment systems addresses via view/pure functions, but eliminate re-entrancy and any attack involving updating contract state.

Speaking to @HardlyDifficult they pointed out that they didn't want to restrict people from tapping into any payment system they wanted, however they wanted. Replacing externalContract.functionCall() with a staticCall provides the best balance of openness and accessibility without significant architectural changes.

Another option would be to require users to register their paymentAddressFactoryCall address and calldata in advance. This would allow the project to blacklist malicious callData and addresses should such activity be identified. The call itself would then have the parameters removed, and instead would reference a mapping lookup.

Another option would be to restrict the feature to certain user classes. Depending on how that restriction was implemented it may not have to form a significant impedence to new users.