Nouns DAO - said's results

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L206-L216 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L231-L234 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L238-L242

Vulnerability details

Impact

DAO Fork voter can drain ERC20 funds by calling quit function, providing the same ERC20 inside erc20TokensToInclude array multiple times, using minimal nouns token amount.

Proof of Concept

If nouns token holders contribute to the fork or join the fork, they can claim the nouns fork token and directly quit the fork to get the eth and ERC20 funds. However, quit function inside NounsDAOLogicV1Fork that provide this mechanism is problematic.

This function let user to choose which ERC20 that they want to get as long as it registered inside the fork token, and they will get the amount based on theirs nouns token amount proportional to total supply. But it doesn't check if ERC20 tokens inside erc20TokensToInclude are unique. Malicious user can provide the same ERC20 token multiple inside the array and get more funds than he supposed to get as long as erc20TokensToInclude inside erc20TokensToIncludeInQuit.

https://github.com/nounsDAO/nouns-monorepo/tree/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance#L206-L216

    function quit(uint256[] calldata tokenIds, address[] memory erc20TokensToInclude) external nonReentrant {
        // check that erc20TokensToInclude is a subset of `erc20TokensToIncludeInQuit`
        address[] memory erc20TokensToIncludeInQuit_ = erc20TokensToIncludeInQuit;
        for (uint256 i = 0; i < erc20TokensToInclude.length; i++) {
            // @audit - only check if it is inside erc20TokensToIncludeInQuit_, not checking duplicate
            if (!isAddressIn(erc20TokensToInclude[i], erc20TokensToIncludeInQuit_)) {
                revert TokensMustBeASubsetOfWhitelistedTokens();
            }
        }

        quitInternal(tokenIds, erc20TokensToInclude);
    }

Inside the quitInternal, the calculation of this arrays of ERC20 token is done, still no check if there is no duplicate.

https://github.com/nounsDAO/nouns-monorepo/tree/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance#L218-L245

    function quitInternal(uint256[] calldata tokenIds, address[] memory erc20TokensToInclude) internal {
        checkGovernanceActive();

        uint256 totalSupply = adjustedTotalSupply();

        for (uint256 i = 0; i < tokenIds.length; i++) {
            nouns.transferFrom(msg.sender, address(timelock), tokenIds[i]);
        }

        uint256[] memory balancesToSend = new uint256[](erc20TokensToInclude.length);

        // Capture balances to send before actually sending them, to avoid the risk of external calls changing balances.
        uint256 ethToSend = (address(timelock).balance * tokenIds.length) / totalSupply;
         // @audit - inputting the same token inside this erc20TokensToInclude allow nouns token fork holder get token more than he deserve
        for (uint256 i = 0; i < erc20TokensToInclude.length; i++) {
            IERC20 erc20token = IERC20(erc20TokensToInclude[i]);
            balancesToSend[i] = (erc20token.balanceOf(address(timelock)) * tokenIds.length) / totalSupply;
        }

        // Send ETH and ERC20 tokens
        timelock.sendETH(payable(msg.sender), ethToSend);
         // @audit - The tokens then send to caller
        for (uint256 i = 0; i < erc20TokensToInclude.length; i++) {
            if (balancesToSend[i] > 0) {
                timelock.sendERC20(msg.sender, erc20TokensToInclude[i], balancesToSend[i]);
            }
        }

        emit Quit(msg.sender, tokenIds);
    }

Nouns fork token holder can get ERC20 token funds more than he deserve, and easily can drain the funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider to also check if the provided erc20TokensToInclude are unique, or don't let user provide the erc20TokensToInclude at all and only depend on quit functions that will process erc20TokensToIncludeInQuit with the provided nouns fork tokenIds.

Assessed type

Invalid Validation