Refactoring NFTMarketCreators.sol can reduce decent amounts of gas and size

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L91-L102
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L154-L162
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L190-L201
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L71-L72
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L134-L135
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L225-L226
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L240-L241

Vulnerability details

Impact

Refactoring NFTMarketCreators.sol can reduce decent amounts of gas and size. Given that the contract size is close to the max(24.576 KB), I will report this as mid pri.

Proof of Concept

_getCreatorPaymentInfo function has some duplicated logic which can be put into function.

Refactoring opportunity #1

Following codes which contain looop are duplicated.
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L91-L102

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L154-L162

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L190-L201

Refactoring opportunity #2

Following codes which resets recipients are duplicated.
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L71-L72

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L134-L135

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L225-L226

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L240-L241

Tools Used

yarn test
yarn size-contracts

Recommended Mitigation Steps

Refactoring opportunity #1

Here is an example function to use at the refactoring opportunity #1.

function _getHasRecipientAndShouldReturn(
    address payable[] memory recipients,
    address seller
) private pure returns (bool hasRecipient, bool shouldReturn) {
    unchecked {
        // The array length cannot overflow 256 bits.
        for (uint256 i = 0; i < recipients.length; ++i) {
        if (recipients[i] != address(0)) {
            hasRecipient = true;
            if (recipients[i] == seller) {
            return (hasRecipient, true);
            }
        }
        }
    }
return (hasRecipient, false);
}

Here is an example code how to use getHasRecipientAndShouldReturn function.

if (_recipients.length == recipientBasisPoints.length) {
    (bool hasRecipient, bool shouldReturn) = _getHasRecipientAndShouldReturn(_recipients, seller);
    if (shouldReturn) {
        return (_recipients, recipientBasisPoints, true);
    }
    if (hasRecipient) {
        recipients = _recipients;
        splitPerRecipientInBasisPoints = recipientBasisPoints;
    }
}

Refactoring opportunity #2

Here is an example function to use at the refactoring opportunity #2.

function _resetAndGetRecipients(
    address newRecipient
) private pure returns (address payable[] memory recipients) {
    recipients = new address payable[](1);
    recipients[0] = payable(newRecipient);
}

Here is an example code how to use _resetAndGetRecipients function.

if (receiver != address(0)) {
    recipients = _resetAndGetRecipients(receiver);
    // splitPerRecipientInBasisPoints is not relevant when only 1 recipient is defined
    if (receiver == seller) {
        return (recipients, splitPerRecipientInBasisPoints, true);
    }
}

Results

Following results are before or after implementing the above two refactoring.

Gas cost

Before the change, $ yarn test returns gas cost for FNDNFTMarket:
Min=5236255
Max=5236291
Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket:
Min=5179631
Max=5179667
Avg=5179663

The above change can reduce 56624 gas cost on average.

Code size

Before the change, $ yarn size-contracts returns 23.877 KB for FNDNFTMarket:
After the change, $ yarn size-contracts returns 23.615 KB for FNDNFTMarket:

The above change can reduce 0.262 KB.

[HardlyDifficult]

Thank you for the suggestion. Yes, this code should be cleaned up. Refactoring with techniques like you suggest would save contract space and help with readability.

Since these suggestions are mostly about improving the contract size, this should have been filed as a gas report. It's true that we are almost at the limit so we are always looking for ways to improve here, making room for new features.

I like the refactors you have suggested here, and it is a non-trivial size savings. We will revisit these in the future. For now, some of the code referenced has been simplified due to related concerns that were raised. Longer term we may switch to handle these calculations in another contract - potentially using https://github.com/manifoldxyz/royalty-registry-solidity/blob/main/contracts/RoyaltyEngineV1.sol or similar, this would save even more space and make our marketplace potentially behave more consistently with others in the space.

[alcueca]

This can be appropriately rewarded even if classified as a gas report. Downgraded as such.

[alcueca]

Included now in #65