Nouns Builder contest - Migue's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L179 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L194 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/storage/MetadataRendererStorageV1.sol#L14-L20

Vulnerability details

Not able to mint token with more than 16 properties.

Proof of Concept

Adding 17 properties via MetadataRenderer.addProperties() method, the mint operation fails with index out of bound error.

It is because attributes property defined at MetadataRendererStorageV1 contract is a mapping from uint256 to fixed array(16): https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/storage/MetadataRendererStorageV1.sol#L20

But property properties is defined as dynamic array. So its length could increase to higher value than 16.

In this escenario, when onMinted() method is executed (during mint process) "index out of bound error" occurs because attributes array elements are acceded based on the numbers of "properties" we have:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L194

script to reproduce (unit testing):

    function test_BrokeMintWith17() public {
    
    deployMock();
    vm.prank(address(treasury));
    string[] memory _names = new string[](17);
    MetadataRendererTypesV1.ItemParam[] memory _items = new MetadataRendererTypesV1.ItemParam[](17);
    
    MetadataRendererTypesV1.IPFSGroup memory _ipfsGroup; 
    _ipfsGroup.baseUri = "";
    _ipfsGroup.extension = "";

    for (uint256 i; i < 17; ++i) {
        _names[i] = ("property_");
        MetadataRendererTypesV1.ItemParam memory _itemParam;
        _itemParam.propertyId = i;
        _itemParam.name = "property_";
        _itemParam.isNewProperty = true;
        _items[i] = _itemParam;
    }

    MetadataRenderer(token.metadataRenderer()).addProperties(_names, _items, _ipfsGroup);
    vm.stopPrank();
    
    vm.prank(address(auction));

    token.mint();
    MetadataRenderer(token.metadataRenderer()).getAttributes(1);
    vm.stopPrank();
    
}

Command to execute the test: forge test --match-test test_BrokeMintWith17

Tools Used

Foundry Manual finding

Recommended Mitigation Steps

I recommend to fit properties array and attributes length to some common length. If so, please consider to change the definition of items declared in Property struct as well: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/types/MetadataRendererTypesV1.sol#L26

Additionally, some business validation must be added in addProperties method in case the provided input have more elements that the allowed.

I do not recommend to get the max length between properties and attributes to fill attributes values. It could case data loss in onMinted() method when "properties" array have more than 16 values. I mean something like that:

uint16 numProperties = properties.length <=16 ? properties.length : 16;

And then iterate the loop.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L182-L194

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L194

Vulnerability details

Impact

It is not possible to mint a ERC721 token if its properties has different length than it's items.

Proof of Concept

I run the following test to reproduce the error:

    deployMock();

    vm.prank(address(governor));
    string[] memory _names = new string[](1);
    _names[0] = "propertyName"; //fill _names with some value
    MetadataRendererTypesV1.ItemParam[] memory _items; //define empty array
    
    MetadataRendererTypesV1.IPFSGroup memory _ipfsGroup; 
    _ipfsGroup.baseUri = "";
    _ipfsGroup.extension = "";
    MetadataRenderer(token.metadataRenderer()).addProperties(_names, _items, _ipfsGroup); //call add property with _items array empty.

    vm.stopPrank();
    vm.prank(address(auction));

    uint256 success = token.mint();//error happens inside here
    assert(success != 0);

    vm.stopPrank();
    

Log from Foundry console:

├─ [736] TOKEN::metadataRenderer() [staticcall]
│   ├─ [353] Token::metadataRenderer() [delegatecall]
│   │   └─ ← METADATA_RENDERER: [0x7076fd06ec2d09d4679d9c35a8db81ace7a07ee2]
│   └─ ← METADATA_RENDERER: [0x7076fd06ec2d09d4679d9c35a8db81ace7a07ee2]
├─ [78618] METADATA_RENDERER::addProperties(["propertyName"], [], ("", ""))
│   ├─ [78172] MetadataRenderer::addProperties(["propertyName"], [], ("", "")) [delegatecall]
│   │   ├─ emit OwnerUpdated(prevOwner: FOUNDER: [0xd3562fd10840f6ba56112927f7996b7c16edfcc1], newOwner: TREASURY: [0xf8cf955543f1ce957b81c1786be64d5fc96ad7b5])      
│   │   ├─ emit PropertyAdded(id: 0, name: "propertyName")
│   │   └─ ← ()
│   └─ ← ()
├─ [0] VM::stopPrank()
│   └─ ← ()
├─ [0] VM::prank(AUCTION: [0x9a1450e42d752b8731bc88f20dbaa9154642f1e6])
│   └─ ← ()
├─ [121037] TOKEN::mint()
│   ├─ [120650] Token::mint() [delegatecall]
│   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: FOUNDER: [0xd3562fd10840f6ba56112927f7996b7c16edfcc1], tokenId: 0)
│   │   ├─ emit DelegateVotesChanged(delegate: FOUNDER: [0xd3562fd10840f6ba56112927f7996b7c16edfcc1], prevTotalVotes: 0, newTotalVotes: 1)
│   │   ├─ [25762] METADATA_RENDERER::onMinted(0)
│   │   │   ├─ [25372] MetadataRenderer::onMinted(0) [delegatecall]
│   │   │   │   └─ ← "Division or modulo by 0"
│   │   │   └─ ← "Division or modulo by 0"
│   │   └─ ← "Division or modulo by 0"
│   └─ ← "Division or modulo by 0"
└─ ← "Division or modulo by 0"

Tools Used

Foundry Manual

Recommended Mitigation Steps

It could be mitigated checking length of both arrays in MetadataRenderer.addProperties() method.

It could be done after those lines: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L111-L115

Also I recommend to move those declaration and new validation at the beginning to save gas.

Governor

Governor.propose

This method calls twice the method proposalThreshold(). It is not necessary because the its value was stored in a variable at the beggining. So you have to replace the second call in line 128 for the variable set before: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L128

Doing that you save ~1772 gas.

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ proposeOld ┆ 9272 ┆ 40259 ┆ 40259 ┆ 71246 ┆ 2 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ propose ┆ 7492 ┆ 38487 ┆ 38487 ┆ 69482 ┆ 2 │

On the other hand, all the following validation must be placed before proposalThreshold() method is called. Doing this, you could save gas when provided data is wrong:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L132-L139

There you are the final optimized method is case you are agree to apply those changes:

/// @notice Creates a proposal
/// @param _targets The target addresses to call
/// @param _values The ETH values of each call
/// @param _calldatas The calldata of each call
/// @param _description The proposal description
function propose(
    address[] memory _targets,
    uint256[] memory _values,
    bytes[] memory _calldatas,
    string memory _description
) external returns (bytes32) {
    // Cache the number of targets
    uint256 numTargets = _targets.length;

    // Ensure at least one target exists
    if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

    // Ensure the number of targets matches the number of values and calldata
    if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
    if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();


    // Get the current proposal threshold
    uint256 currentProposalThreshold = proposalThreshold();

    // Cannot realistically underflow and `getVotes` would revert
    unchecked {
        // Ensure the caller's voting weight is greater than or equal to the threshold
        if (getVotes(msg.sender, block.timestamp - 1) < currentProposalThreshold) revert BELOW_PROPOSAL_THRESHOLD();
    }

    // Compute the description hash
    bytes32 descriptionHash = keccak256(bytes(_description));

    // Compute the proposal id
    bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

    // Get the pointer to store the proposal
    Proposal storage proposal = proposals[proposalId];

    // Ensure the proposal doesn't already exist
    if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

    // Used to store the snapshot and deadline
    uint256 snapshot;
    uint256 deadline;

    // Cannot realistically overflow
    unchecked {
        // Compute the snapshot and deadline
        snapshot = block.timestamp + settings.votingDelay;
        deadline = snapshot + settings.votingPeriod;
    }

    // Store the proposal data
    proposal.voteStart = uint32(snapshot);
    proposal.voteEnd = uint32(deadline);
    proposal.proposalThreshold = uint32(currentProposalThreshold);
    proposal.quorumVotes = uint32(quorum());
    proposal.proposer = msg.sender;
    proposal.timeCreated = uint32(block.timestamp);

    emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

    return proposalId;
}

---

---

---

Governor

Governor.proposalVotes

I have added a POC for this method and realized that if you avoid to declare the memory variable and get the values from storage is cheaper (~412 gas) than the already coded approach. I assume that it is a sort of compiler's optimization.

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ proposalVotes ┆ 759 ┆ 759 ┆ 759 ┆ 759 ┆ 3 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ proposalVotesOld ┆ 1171 ┆ 1171 ┆ 1171 ┆ 1171 ┆ 1 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

There you are the optimized method to use in case you consider properly:

/// @notice The vote counts for a proposal
/// @param _proposalId The proposal id
function proposalVotes(bytes32 _proposalId)
    external
    view
    returns (
        uint256,
        uint256,
        uint256
    )
{
    return (proposals[_proposalId].againstVotes, proposals[_proposalId].forVotes, proposals[_proposalId].abstainVotes);
}

---

---

---

Token.mint()

Token._isForFounder(uint256)

_isForFounder(uint256) method could be optimized storing in memory the value of tokenRecipient[baseTokenId]. It is read from storage three times. Doing this optimization you will save ~260 gas during mint process:

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ mintOld ┆ 298756
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ mint ┆ 298496 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

You could use the following code in case think the optimization is valid:

/// @dev Checks if a given token is for a founder and mints accordingly
/// @param _tokenId The ERC-721 token id
function _isForFounder(uint256 _tokenId) private returns (bool) {
    // Get the base token id
    uint256 baseTokenId = _tokenId % 100;

    // If there is no scheduled recipient:
    Founder memory _founder = tokenRecipient[baseTokenId];
    if (_founder.wallet == address(0)) {
        return false;

        // Else if the founder is still vesting:
    } else if (block.timestamp < _founder.vestExpiry) {
        // Mint the token to the founder
        _mint(_founder.wallet, _tokenId);

        return true;

        // Else the founder has finished vesting:
    } else {
        // Remove them from future lookups
        delete tokenRecipient[baseTokenId];

        return false;
    }
}