code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L80-L99 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L124 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L251-L265

Vulnerability details

Impact

During initialization of Token.sol (more concrete during Token._addFounders), data of founders who have zero ownership percentage (which can be a reasonable case, if someone wants to be an honorary founder without compensation), is not stored correctly. The external function Token.getFounders will return the right number of structs, but there will be empty ones for each founder with zero ownership percentage. This could potentially affect future use cases that require the founders wallet data, which a DAO or the founders want to implement.

Proof of Concept

Token.sol, L. 85:

if (founderPct == 0) continue;

This skips the initialization and storage of founder data that follows afterwards:

Token.sol, L. 94-99:

Founder storage newFounder = founder[founderId];

// Store the founder's vesting details
newFounder.wallet = _founders[i].wallet;
newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
newFounder.ownershipPct = uint8(founderPct);

Token.sol, L. 124:

settings.numFounders = uint8(numFounders);

This stores the correct number of founders and is used in Token.sol, L. 251-265 to retrieve the founders data:

function getFounders() external view returns (Founder[] memory) {
    // Cache the number of founders
    uint256 numFounders = settings.numFounders;

    // Get a temporary array to hold all founders
    Founder[] memory founders = new Founder[](numFounders);

    // Cannot realistically overflow
    unchecked {
        // Add each founder to the array
        for (uint256 i; i < numFounders; ++i) founders[i] = founder[i];
    }

    return founders;
}

Tools Used

The POC has been verified by modifying the existing test cases and retrieving the founders data.

Recommended Mitigation Steps

Move the line if (founderPct == 0) continue; after L. 99 to ensure founder data is stored.

GalloDaSballo commented 2 years ago

Dup of #411