Nouns Builder contest - 0x4non's results

Lines of code

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

Vulnerability details

Impact

Is possible to generate an underflow on ERC721Votes.sol#L216 mainly because its wrapped in an unchecked bracked.

prevTotalVotes could be lower than _amount so this will generate and underflow; _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);

Proof of Concept

If user A delegates votes to B delegate(B) then if nCheckpoints is 0 it will underflow, and also if prevTotalVotes is lower than _amount it will underflow.

Tools Used

Manual revision

Recommended Mitigation Steps

First of all i think its a good practice to document use of unchecked. Concretely describe why it is safe to not perform arithmetic checks on each code block. Preferably for each operation. In this particular case you could add a require to check if the operation is safe require(prevTotalVotes >= _amount, "unsafe math");;

    function _moveDelegateVotes(
        address _from,
        address _to,
        uint256 _amount
    ) internal {
        unchecked {
            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                if (_from != address(0)) {
                    // Get the sender's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_from]++;

                    // Used to store the sender's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the sender's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

                    require(prevTotalVotes >= _amount, "unsafe math");

                    // Update their voting weight
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L79-L80 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L583 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L591

Vulnerability details

Impact

Variables proposalThresholdBps and quorumThresholdBps can be type(uint16).max The definition of BPS is;

Basis points, otherwise known as bps or "bips," are a unit of measure used in finance to describe the percentage change in the value of financial instruments or the rate change in an index or other benchmark. One basis point is equivalent to 0.01% (1/100th of a percent) or 0.0001 in decimal form.

Max value of BPS should be 10_000

Proof of Concept

Owner can call updateQuorumThresholdBps or updateProposalThresholdBpswith a value greater than 10_000

Tools Used

Manual revision

Recommended Mitigation Steps

Add a require to assert that proposalThresholdBps and quorumThresholdBps are in a valid range on update or initialize phase.

QA

Variables _owner and _pendingOwner should be private instead of internal

Variables _owner and _pendingOwner should be private instead of internal to avoid other contracts messing around with this variables; Ownable.sol#L17-L21

---

Avoid shadow variables

Shadow variable from Ownable.sol#L18

diff --git a/src/manager/Manager.sol b/src/manager/Manager.sol
index d1025ec..86a94e5 100644
--- a/src/manager/Manager.sol
+++ b/src/manager/Manager.sol
@@ -76,13 +76,13 @@ contract Manager is IManager, UUPS, Ownable, ManagerStorageV1 {
     ///                                                          ///
 
     /// @notice Initializes ownership of the manager contract
-    /// @param _owner The owner address to set (will be transferred to the Builder DAO once its deployed)
-    function initialize(address _owner) external initializer {
+    /// @param owner_ The owner address to set (will be transferred to the Builder DAO once its deployed)
+    function initialize(address owner_) external initializer {
         // Ensure an owner is specified
-        if (_owner == address(0)) revert ADDRESS_ZERO();
+        if (owner_ == address(0)) revert ADDRESS_ZERO();
 
         // Set the contract owner
-        __Ownable_init(_owner);
+        __Ownable_init(owner_);
     }
 
     ///                                                          ///

---

Unsafe downcasting to _newTotalVotes on _writeCheckpoint method ERC721Votes.sol#L252

If _newTotalVotes are greater than type(uint192).max they will be downcast to type(uint192).max

---

The function registerUpgrade(address _baseImpl, address _upgradeImpl) should check that _upgradeImpl is a contract https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L187

---

Private variables should start with underscore

On Token.sol#L23 private variable manager should be _manager

---

Interfaces filename should have I prefix

So MetadataRendererTypesV1.sol should be IMetadataRendererTypesV1.sol

---

Interfaces should have open pragma

IManager.sol#L2 ITreasury.sol#L2 IToken.sol#L2 IGovernor.sol#L2 IBaseMetadata.sol#L2 IPropertyIPFSMetadataRenderer.sol#L2 MetadataRendererTypesV1.sol#L2 IAuction.sol#L2

---

Remove unused function

This functions are never used

diff --git a/src/lib/utils/Address.sol b/src/lib/utils/Address.sol
index 3447bc1..ec327cb 100644
--- a/src/lib/utils/Address.sol
+++ b/src/lib/utils/Address.sol
@@ -21,10 +21,6 @@ library Address {
     ///                           FUNCTIONS                      ///
     ///                                                          ///
 
-    /// @dev Utility to convert an address to bytes32
-    function toBytes32(address _account) internal pure returns (bytes32) {
-        return bytes32(uint256(uint160(_account)) << 96);
-    }
 
     /// @dev If an address is a contract
     function isContract(address _account) internal view returns (bool rv) {

Also Remove Initializable.sol:_disableInitializers()

---

Gas report

Use cached _auction varible instead of state variable.

diff --git a/src/auction/Auction.sol b/src/auction/Auction.sol
index 794da99..3754f3b 100644
--- a/src/auction/Auction.sol
+++ b/src/auction/Auction.sol
@@ -169,7 +169,7 @@ contract Auction is IAuction, UUPS, Ownable, ReentrancyGuard, Pausable, AuctionS
         Auction memory _auction = auction;
 
         // Ensure the auction wasn't already settled
-        if (auction.settled) revert AUCTION_SETTLED();
+        if (_auction.settled) revert AUCTION_SETTLED();
 
         // Ensure the auction had started
         if (_auction.startTime == 0) revert AUCTION_NOT_STARTED();

---

Use Reentrancy guard based on solmate implementation, plus use uint32 instead of uint256, overall savings;

Overall gas change: -475255 (-1.036%) Changes;

diff --git a/src/lib/utils/ReentrancyGuard.sol b/src/lib/utils/ReentrancyGuard.sol
index 8c6dd15..7afec50 100644
--- a/src/lib/utils/ReentrancyGuard.sol
+++ b/src/lib/utils/ReentrancyGuard.sol
@@ -10,14 +10,8 @@ abstract contract ReentrancyGuard is Initializable {
     ///                            STORAGE                       ///
     ///                                                          ///
 
-    /// @dev Indicates a function has not been entered
-    uint256 internal constant _NOT_ENTERED = 1;
-
-    /// @dev Indicates a function has been entered
-    uint256 internal constant _ENTERED = 2;
-
     /// @notice The reentrancy status of a function
-    uint256 internal _status;
+    uint32 private _status;
 
     ///                                                          ///
     ///                            ERRORS                        ///
@@ -32,17 +26,19 @@ abstract contract ReentrancyGuard is Initializable {
 
     /// @dev Initializes the reentrancy guard
     function __ReentrancyGuard_init() internal onlyInitializing {
-        _status = _NOT_ENTERED;
+        // 1 = _NOT_ENTERED
+        _status = 1;
     }
 
     /// @dev Ensures a function cannot be reentered
     modifier nonReentrant() {
-        if (_status == _ENTERED) revert REENTRANCY();
-
-        _status = _ENTERED;
+        if (_status == 2) revert REENTRANCY();
+        // 2 = _ENTERED
+        _status = 2;
 
         _;
 
-        _status = _NOT_ENTERED;
+        // 1 = _NOT_ENTERED
+        _status = 1;
     }
 }

---

Inline modifier (used only once)

You are using the modifier onlyPendingOwner only once, insteaf of create a modifier just add a check on acceptOwnershipto save gas

diff --git a/src/lib/utils/Ownable.sol b/src/lib/utils/Ownable.sol
index 04b5854..8de8863 100644
--- a/src/lib/utils/Ownable.sol
+++ b/src/lib/utils/Ownable.sol
@@ -30,12 +30,6 @@ abstract contract Ownable is IOwnable, Initializable {
         _;
     }
 
-    /// @dev Ensures the caller is the pending owner
-    modifier onlyPendingOwner() {
-        if (msg.sender != _pendingOwner) revert ONLY_PENDING_OWNER();
-        _;
-    }
-
     ///                                                          ///
     ///                           FUNCTIONS                      ///
     ///                                                          ///
@@ -75,7 +69,8 @@ abstract contract Ownable is IOwnable, Initializable {
     }
 
     /// @notice Accepts an ownership transfer
-    function acceptOwnership() public onlyPendingOwner {
+    function acceptOwnership() public {
+        if (msg.sender != _pendingOwner) revert ONLY_PENDING_OWNER();
         emit OwnerUpdated(_owner, msg.sender);
 
         _owner = _pendingOwner;

---

These funcions can be declared as external to save gas;

cancelOwnershipTransfer