returnTokensToOwner() in NounsDAOForkEscrow.sol can revert unexpectedly due to improper subtraction

[code423n4]

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/NounsDAOForkEscrow.sol#L116 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/NounsDAOForkEscrow.sol#L124

Vulnerability details

Impact

returnTokensToOwner() in NounsDAOForkEscrow.sol can revert unexpectedly because of the numTokensInEscrow -= tokenIds.length; math in its logic. tokenIds is an externally supplied argument and it can be larger than the storage variable numTokensInEscrow. Subtraction of larger uint value from smaller uint value can cause a panic/unexpected revert in solidity.

Proof of Concept

    function returnTokensToOwner(address owner, uint256[] calldata tokenIds) external onlyDAO {
        for (uint256 i = 0; i < tokenIds.length; i++) {
            if (currentOwnerOf(tokenIds[i]) != owner) revert NotOwner();

            nounsToken.transferFrom(address(this), owner, tokenIds[i]);
            escrowedTokensByForkId[forkId][tokenIds[i]] = address(0);
        }

        numTokensInEscrow -= tokenIds.length;
    }

Tools Used

vs code

Recommended Mitigation Steps

add checks to prevent subtraction if numTokensInEscrow < tokenIds.length

Assessed type

Math

[0xSorryNotSorry]

if numTokensInEscrow < tokenIds.length the function will revert.

Invalid assumption

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid