Nouns Builder contest - scaraven's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L262-L268 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L189

Vulnerability details

Impact

In ERC721Votes.sol, when an ERC721 token is minted and transferred to a user who has delegated their tokens, the token voting weight is incorrectly added to the voting weight of the user in _afterTokenTransfer(). This means that tokens are inconsistently delegated, as part of the user's tokens are delegated towards themselves and the rest to another user. When a user then tries to change their delegation through delegate(), the contract calls balanceOf() and assumes all user tokens are delegated to the delegatee which can cause an underflow and allow a user to gain a very large number of votes.

This can be exploited to vote for malicious proposals or DOS the governance and vote against all proposals.

High severity is suitable as this can be done by any user at any time and allows a user to gain complete control over the governance protocol

Proof of Concept

1. Alice buys token#1 and delegates it to Bob
2. Alice buys token#2
3. Auction.sol calls token.transferFrom(address(this), alice.address, tokenId);
4. The balances are updated and _afterTokenTransfer() is called which then subsequently calls _moveDelegateVotes(_from, _to, 1); where _to is Alice's address
5. Alice and Bob now have a vote each when Bob should instead have both
6. Alice calls delegate(_to) => _moveDelegateVotes(prevDelegate, _to, balanceOf(_from);
7. As Alice's balance is 2, Bob's weight underflows (as unchecked is used) and becomes type(uint256).max

Tools Used

VS Code

Recommended Mitigation Steps

Modify _afterTokenTransfer() to:

    function _afterTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal override {
        //if sender or recipient have not delegated, then automatically self-delegate
        if(delegation[_from] == address(0) && _from != address(0)) {
            delegation[_from] = _from;
        }
        if(delegation[_to] == address(0) &&  _to != address(0)) {
            delegation[_to] = _to;
        }
        // Transfer 1 vote from the sender's delegatee to the recipient's delegatee
        _moveDelegateVotes(delegation[_from], delegation[_to], 1);


        super._afterTokenTransfer(_from, _to, _tokenId);
    }

Additionally, ensure that a user is not able to delegate to address(0) as that will break the above logic.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L151-L157

Vulnerability details

Impact

When the total founder ownership is 100%, all NFT tokens which are minted will be automatically minted for the founders. However, as token.mint() will constantly increase tokenID, unbounded gas consumption occurs which will cause the external call to revert due to exceeding block gas limits, thereby pausing the auction contract without the founders receiving any NFTs. Therefore, the auction will only properly work once the founder's claim has expired, with founders not receiving a single NFT.

I believe medium severity is suitable as this issue breaks a pretty significant selling point of the protocol however requires that the founders setup the governance with 100% initial ownership

Proof of Concept

1. Alice deploys the new governance protocol and sets founderPct = 100
2. Alice starts the auction by calling unpause() in Auction.sol
3. _createAuction() calls token.mint() which starts an infinite loop and reverts
4. Due to the try/catch statement, the contract is paused and no auction can start

Tools Used

VS Code

Recommended Mitigation Steps

Add a check to make sure that founder ownership does not reach 100% e.g. In _addFounders() from Token.sol add a greater or equal than sign:

if ((totalOwnership += uint8(founderPct)) >= 100) revert INVALID_FOUNDER_OWNERSHIP();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L118 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L179

Vulnerability details

Impact

Due to improperly updating baseTokenId in _addFounders(), baseTokenId may grow larger than 100 which means subsequently allocated NFTs are not properly rewarded to the founders. This can cause some founders to receive drastically fewer NFTs and voting rights than they should have.

Proof of Concept

1. Let's assume we have two founders called Alice and Bob who agreed to allocated themselves 51% and 49% of the protocol respectively
2. Assuming Alice is the first founder, she will be assigned baseTokenIds : 0, 1, ... , 49, 50 because schedule rounds down to 100 / 51 = 1
3. Bob's schedule rounds down to 100 / 49 = 2 and receives baseTokenId: 51, 53, ... , 99, ... , 147
4. However, when tokens are assigned in isForFounder(), baseTokenId is assigned using:

uint256 baseTokenId = _tokenId % 100;

Thereby ensuring that 0 <= baseTokenId <= 99. Therefore, all baseTokenIds assigned to Bob which are larger than 100 is useless, meaning that Bob receives only 25% of all tokens instead of his 49%

Tools Used

VS Code

Recommended Mitigation Steps

Properly update baseTokenId i.e. Change

(baseTokenId += schedule) % 100;

to

baseTokenId = (baseTokenId + schedule) % 100;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L210-L217

Vulnerability details

Impact

A founder can rug pull governance by creating a malicious "sleeper" proposal with a very large delay to ensure that it does not expire. When the proposal is ready for execution, the founder can execute it and steal all funds stored in the treasury. Through this method, a founder can create the malicious proposal and then make legitimate and seemingly trustworthy actions such as reducing their voting weight, updating the vetoer to a trusted third party (multi-sig wallet or governance) etc...

I believe medium severity is suitable as this can cause substantial loss of user funds, however requires people to buy NFTs from the malicious governance. Considering that unprivileged attackers have been able to sneak past malicious proposals to reck governance protocols in the past (even with vetoers), this situation is not far-fetched.

Proof of Concept

1. A founder deploys a new governance protocol and gives themselves enough voting weight to create a proposal
2. the founder increases the treasury settings.delay through updateDelay() to be a very large period of time (1 year, 2 years etc...)
3. the founder creates a proposal which drains a prespecified amount of ETH from the contract and passes it
4. the founder resets updateDelay() to a normal period e.g. 1 week and then voluntarily gives up the majority of their power to gain trust
5. Once the delay on the founder's proposal is over, the founder can now queue and execute the malicious proposal

Tools Used

VS Code

Recommended Mitigation Steps

I recommend adding functionality which requires a certain number of votes to approve execution of a proposal so that proposals which might not be otherwise checked are given proper attention

This means that users who have a stake in governance now have control over proposals which may have been proposed a very long time ago which they had no control over

Lines of code

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

Vulnerability details

Impact

When token.mint() is called at the creation of an auction, Token.sol makes an external call with MetadataRenderer.onMinted() which generates the attributes for the specific token using a seed. The attributes of the token is stored in a static array of length 16 called tokenAttributes and the function then goes on to loop through each property and assign one item from each property to the static array. If the number of properties is larger than 15 (as index 0 is used to store numProperties) then the function will revert due to out of bounds access. This would mean that minting tokens will always fail and Auction.sol will stop working.

I believe medium severity is suitable, as this can permanently or temporarily break the protocol if the number of properties is above 15 (external circumstances).

Proof of Concept

1. Founder deploys governance protocol and adds 16 properties
2. Founder starts auction through auction.unpause() and a token is minted through token.mint()
3. In _mint(), the contract makes an external call to metadataRenderer.onMinted() which fails due to out of bounds access
4. Auction is paused and protocol now cannot auction NFTs

Tools Used

VS Code

Recommended Mitigation Steps

There are two possible solutions:

1. Add a limit on the number of properties which can be added in metadataRenderer.addProperties() e.g.

if(numStoredProperties + numNewProperties > 15) revert TOO_MANY_PROPERTIES();

2. Change tokenAttributes to a dynamic array

The second option is more preferable as it allows more flexibility for founders which want to create a protocol with many properties.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L116 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L98-L105 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L353

Vulnerability details

Impact

In hashProposal() from Governer.sol, the generated proposalID is independent of msg.sender and can be generated by anyone. Therefore, if a user tries to create a new proposal through propose() an attacker can frontrun the transaction where they call propose() with the same call data and then call cancel(). As the contract does not allow duplicate propsalID, the user's proposal will not be registered.

I believe medium severity is suitable as it can cause in temporary or permanent failure of the protocol however requires that the attacker can frontrun a user transaction

Proof of Concept

1. A tx where a user calls propose() is added to the public mempool
2. An attacker sees the transaction in the public mempool and creates a new tx with the same calldata but with a higher gas price
3. The attacker's transaction is run first thereby creating the proposal and setting the auction.proposer to the attacker's address
4. To prevent the proposal from going ahead, the attacker can call cancel()

Tools Used

VS Code

Recommended Mitigation Steps

In hashProposal(), add msg.sender so that any proposalID is unique to that user e.g.

    function hashProposal(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        bytes32 _descriptionHash
    ) public pure returns (bytes32) {
        return keccak256(abi.encode(msg.sender, _targets, _values, _calldatas, _descriptionHash));
    }

Judge has assessed an item in Issue #593 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L21 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Ownable.sol#L63 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L22

Vulnerability details

Impact

governor.transferOwnership() and Auction.transferOwnership() is exposed to Treasury.sol and can be called in a governance proposal. If this is used then it causes an inconsistency between the owner of these contracts and settings.treasury which would prevent governance from ever modifying its own parameters and Ethereum from auctions will be sent to the wrong address.

I believe medium severity is suitable because we lose partial control of governance and/or ETH from auctions, assuming that a proposal (malicious or not) is passed which changes contract ownership.

Proof of Concept

1. A proposal which calls governor.transferOwnership() is executed and transfers ownership to a new treasury address
2. When subsequent proposals are passed, Governor.sol will make an external call to settings.treasury which is not modified when ownership is passed
3. Therefore, settings.treasury is not able to modify any governance parameters such as proposalThreshold

The same issue applies to Auction.sol

Tools Used

VS Code

Recommended Mitigation Steps

Modify transferOwnership() in the relevant contracts so that if ownership is transferred, then settings.treasury is changed accordingly.

Issue 1 Lack of zero checks on founder address

When looping through each founder parameters in Token._addFounders(), there is no check that a founder address is address(0), this can cause inconsistencies between OwnershipPct and the actual percentage of NFTs which have been minted

Consider adding a check to make sure that a founder address is not 0

Issue 2 founderPct may be larger than 100

Due to inconsistently casting founderPct to uint(8), founderPct may be truncated and pass all verification checks however when the baseTokenIDs is assigned, the function through loops through the full value which can cause the founder to receive full ownership of all NFTs even if OwnershipPct is set to 0.

Consider adding a check to make sure that founderPct < type(uint8).max