Nouns DAO - iglyx's results

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/e439acb302523451034f2cbb1379e0793bff41a6/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L206-L216

Vulnerability details

quit() doesn't check erc20TokensToInclude argument for repetitions (i.e. token address duplications), only checking the existence of a token in erc20TokensToIncludeInQuit. Each time a token repeats the corresponding share of treasury holdings will be transferred to the quitting member.

Impact

The whole ERC20 holdings can be stolen by any forked DAO nouns holder.

Proof of Concept

Suppose Bob is the holder of 1 of the 50 forked nouns and stETH is the only meaningful token on the balance, and it is included to erc20TokensToIncludeInQuit. Bob can submit erc20TokensToInclude = [address(stETH), ..., address(stETH)] (50 times in total), which will pass the check:

https://github.com/nounsDAO/nouns-monorepo/blob/e439acb302523451034f2cbb1379e0793bff41a6/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#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++) {
            if (!isAddressIn(erc20TokensToInclude[i], erc20TokensToIncludeInQuit_)) {  // @audit this check will be passed 50 times
                revert TokensMustBeASubsetOfWhitelistedTokens();
            }
        }

        quitInternal(tokenIds, erc20TokensToInclude);
    }

And run quitInternal() with this array, performing 50 transfers of 1/50 share each:

https://github.com/nounsDAO/nouns-monorepo/blob/e439acb302523451034f2cbb1379e0793bff41a6/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#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;
        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);
        for (uint256 i = 0; i < erc20TokensToInclude.length; i++) {
            if (balancesToSend[i] > 0) {
                timelock.sendERC20(msg.sender, erc20TokensToInclude[i], balancesToSend[i]);  // @audit this will transfer 1/50 of the holdings 50 times
            }
        }

        emit Quit(msg.sender, tokenIds);
    }

Recommended Mitigation Steps

A simple way to handle this is to add checkForDuplicates(erc20tokens) check to the corresponding quit() function:

https://github.com/nounsDAO/nouns-monorepo/blob/e439acb302523451034f2cbb1379e0793bff41a6/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L206-L216

    function quit(uint256[] calldata tokenIds, address[] memory erc20TokensToInclude) external nonReentrant {
+       checkForDuplicates(erc20TokensToInclude);
        // check that erc20TokensToInclude is a subset of `erc20TokensToIncludeInQuit`
        address[] memory erc20TokensToIncludeInQuit_ = erc20TokensToIncludeInQuit;
        for (uint256 i = 0; i < erc20TokensToInclude.length; i++) {
            if (!isAddressIn(erc20TokensToInclude[i], erc20TokensToIncludeInQuit_)) {  // @audit this check will be passed 50 times
                revert TokensMustBeASubsetOfWhitelistedTokens();
            }
        }

        quitInternal(tokenIds, erc20TokensToInclude);
    }

Assessed type

Invalid Validation