Mitigation/trust-audit-spencer by spengrah · Pull Request #97 · Hats-Protocol/hats-protocol

spengrah · 2023-02-20T20:14:49Z

topocount

Awesome! I have a handful of suggested changes and questions that optionally could lead to a change, but this looks fantastic

topocount · 2023-02-22T02:19:45Z

src/HatsIdUtilities.sol

+        if (_hatId & uint256(type(uint224).max) == 0) return 0;
+        if (_hatId & uint256(type(uint208).max) == 0) return 1;
+        if (_hatId & uint256(type(uint192).max) == 0) return 2;
+        if (_hatId & uint256(type(uint176).max) == 0) return 3;
+        if (_hatId & uint256(type(uint160).max) == 0) return 4;
+        if (_hatId & uint256(type(uint144).max) == 0) return 5;
+        if (_hatId & uint256(type(uint128).max) == 0) return 6;
+        if (_hatId & uint256(type(uint112).max) == 0) return 7;
+        if (_hatId & uint256(type(uint96).max) == 0) return 8;
+        if (_hatId & uint256(type(uint80).max) == 0) return 9;
+        if (_hatId & uint256(type(uint64).max) == 0) return 10;
+        if (_hatId & uint256(type(uint48).max) == 0) return 11;
+        if (_hatId & uint256(type(uint32).max) == 0) return 12;
+        if (_hatId & uint256(type(uint16).max) == 0) return 13;


not sure if hardcoding these max valueswill improve runtime cost, but I'm guessing it will.

topocount · 2023-02-22T02:21:01Z

README.md

 Hats Protocol is a protocol for DAO-native roles and credentials that supports revocable delegation of authority and responsibility.

-Hats are represented on-chain by non-transferable ERC1155 tokens. An address with a balance of a given Hat token "wears" that hat, granting them the responsibilities and authorities that have been assigned to the Hat by the DAO.
+Hats are represented on-chain by non-transferable tokens that conform to the ERC1155 interface. An address with a balance of a given Hat token "wears" that hat, granting them the responsibilities and authorities that have been assigned to the Hat by the DAO.


nice technicality here

topocount · 2023-02-22T02:25:38Z

README.md

+
+Hats Protocol conforms fully to the ERC1155 interface. All external functions required by the [ERC1155 standard](https://eips.ethereum.org/EIPS/eip-1155) are exposed by Hats Protocol. This is how Hats can work out of the box with existing token-gating applications.
+
+However, Hats Protocol is not fully compliant with the ERC1155 standard. Since Hats are not transferable by their owners (aka "wearers"), there is little need for safe transfers and the `ERC1155TokenReceiver` logic. Developers building on top of Hats Protocol should note that mints and transfers of Hats will not, for example, include calls to `onERC1155Received`.


The special case here is smart contracts that administer hats. However, if a smart contract is capable of interfacing with the hats contract, it's probably capable of burning or reassigning any self-assigned hats. Probably not worth documenting, just trying to be explicit here.

topocount · 2023-02-22T02:27:03Z

README.md

 Only a Hat's admin can mint its token to a wearer.

-To mint a Hat, the Hat's max supply must not have already been reached, and the target wearer must not already wear the Hat.
+To mint a Hat, the Hat's max supply must not have already been reached, the target wearer must not already wear the Hat, and the target wearer must not be eligible for the hat.


wait...shouldn't the target be eligible?

whoops, typo — good catch!

topocount · 2023-02-22T02:29:19Z

src/HatsIdUtilities.sol

+    /// @dev Similar to getHatLevel, but does not account for linked trees
+    /// @param _hatId the id of the hat in question
+    /// @return level The local level, from 0 to 14
+    function getLocalHatLevel(uint256 _hatId) public pure returns (uint32 level) {


sorry to be pedantic, but isn't it more self-descriptive to use getTreeHatLevel? I'm just inquiring and will leave this for your discretion

Heh I went back and forth on this too. I was inspired by your variable names in getAdminAtLevel, and we use tree in a number of different contexts so I think Local is currently clearer all things considered.

plus apparently @nintynick likes it 😉

topocount · 2023-02-22T02:39:53Z

src/Hats.sol

+        */
+        if (success && returndata.length == 32) {
+            // check the returndata manually
+            uint256 uintReturndata = uint256(bytes32(returndata));


I'm guessing this is cheaper then abi.decode? I'd imagine so, since it's potentially lossy and has no safety checks built in.

actually, its ever so slightly more expensive. Changing to abi.decode...

topocount · 2023-02-22T02:51:31Z

src/Hats.sol

+        if (success && returndata.length == 64) {
            bool standing;
-            (eligible, standing) = abi.decode(returndata, (bool, bool));
-            // never eligible if in bad standing
-            if (eligible && !standing) eligible = false;
+            // check the returndata manually
+            (uint256 firstWord, uint256 secondWord) = abi.decode(returndata, (uint256, uint256));
+            // returndata is valid
+            if (firstWord < 2 && secondWord < 2) {
+                standing = (secondWord == 1) ? true : false;
+                // never eligible if in bad standing
+                eligible = (standing && firstWord == 1) ? true : false;
+            }
+            // returndata is invalid
+            else {
+                eligible = !badStandings[_hatId][_wearer];
+            }


I agree that this should be deduplicated in a helper function.

topocount · 2023-02-22T02:52:35Z

src/Hats.sol

+        if (success && returndata.length == 32) {
+            // check the returndata manually
+            uint256 uintReturndata = uint256(bytes32(returndata));
+            // false condition
+            if (uintReturndata == 0) {
+                active = false;
+                // true condition
+            } else if (uintReturndata == 1) {
+                active = true;
+            }
+            // invalid condition
+            else {
+                active = _getHatStatus(_hat);
+            }
        } else {
-            return _getHatStatus(_hat);
+            active = _getHatStatus(_hat);


If solidity supported some light metaprogramming, we could deduplicate this codeblock but I think it's too nuanced.

I actually did try...

_retrieveAndProcessToggleReturnData(Hat memory _hat, uint256 _hatId, function(Hat memory) internal view returns (bool) _fallbackFunc) internal view returns (bool)

But it ended up being more expensive on execution so I stuck with the repeated code, as messy as it looks.

topocount · 2023-02-22T03:02:52Z

src/Hats.sol

+        } else {
+            if (isWearerOfHat(_user, linkedTreeAdmin)) return true; // user wears the linkedTreeAdmin
+
+            else return isAdminOfHat(_user, linkedTreeAdmin); // check if user is admin of linkedTreeAdmin (recursion)


It's crazy to me that all this extra logic saves us gas relative to an extra storage read for linkages, but here we are! Great work sorting this out. I didn't realize this, on top of the HatsIdUtils library refactor was needed, and feel bad kicking this back to you now.

Not all all! Was fun to untangle :)

topocount · 2023-02-22T03:04:33Z

src/Hats.sol

+        if (isLocalTopHat(_hatId)) {
+            linkedTreeAdmin = linkedTreeAdmins[getTophatDomain(_hatId)];
+            if (linkedTreeAdmin == 0) {
+                // tree is not linked
+                return isWearerOfHat(_user, _hatId);
+            } else {
+                // tree is linked
+                if (isWearerOfHat(_user, linkedTreeAdmin)) {
+                    return true;
+                } // user wears the treeAdmin
+                else {
+                    adminLocalHatLevel = getLocalHatLevel(linkedTreeAdmin);
+                    _hatId = linkedTreeAdmin;
+                }
+            }


This is a nice short-circuit

nintynick

Reviewed alongside the audit report and all the changes look great. Left specific review comments, mostly editorial.

nintynick · 2023-02-22T02:33:59Z

README.md

-<p align="right">(<a href="#documentation-top">back to contents</a>)</p>
+<p align="right">(<a href="#table-of-contents">back to contents</a>)</p>
+
+### ERC1155 Compatibility


well done on this section

nintynick · 2023-02-22T02:34:24Z

README.md

 | --- | ---- |
-| Signer on a multisig | Using the Hat's ERC1155 token as a condition for membership in an Orca Protocol Pod |
-| Admin of the DAO's Github repo | Using the Hat's ERC1155 token as a condition for access via Lit Protocol |
+| Signer on a multisig | Using the Hat's ERC1155--similar token as a condition for membership in an Orca Protocol Pod |


Suggested change

| Signer on a multisig | Using the Hat's ERC1155--similar token as a condition for membership in an Orca Protocol Pod |

| Signer on a multisig | Using the Hat's ERC1155-similar token as a condition for membership in an Orca Protocol Pod |

nintynick · 2023-02-22T02:34:59Z

src/Hats.sol


 import { ERC1155 } from "lib/ERC1155/ERC1155.sol";
-// import "forge-std/Test.sol"; //remove after testing
+// import { console2 } from "forge-std/Test.sol"; //remove after testing


yea, will remove before deploy

nintynick · 2023-02-22T02:35:20Z

src/Hats.sol

-/// @notice Hats are DAO-native, revocable, and programmable roles that are represented as non-transferable ERC-1155 tokens for composability
-/// @dev This is a multitenant contract that can manage all hats for a given chain
+/// @notice Hats are DAO-native, revocable, and programmable roles that are represented as non-transferable ERC-1155-similar tokens for composability
+/// @dev This is a multitenant contract that can manage all hats for a given chain. While it fully implements the ERC1155 interface, it does not comply full with the ERC1155 standard.


Suggested change

/// @dev This is a multitenant contract that can manage all hats for a given chain. While it fully implements the ERC1155 interface, it does not comply full with the ERC1155 standard.

/// @dev This is a multitenant contract that can manage all hats for a given chain. While it fully implements the ERC1155 interface, it does not fully comply with the ERC1155 standard.

nintynick · 2023-02-22T02:35:31Z

src/Hats.sol

+        /* 
+        if function call succeeds with data of length == 32, then we know the contract exists 
+        and has the getHatStatus function.
+        But we still can't assume that the return data is a boolean, so we need to check that manuallys


Suggested change

But we still can't assume that the return data is a boolean, so we need to check that manuallys

But we still can't assume that the return data is a boolean, so we need to check that manually

nintynick · 2023-02-22T03:36:48Z

src/Hats.sol

+            // false condition
+            if (uintReturndata == 0) {
+                active = false;
+                // true condition


Suggested change

// true condition

nintynick · 2023-02-22T03:37:14Z

src/Hats.sol

+            if (uintReturndata == 0) {
+                active = false;
+                // true condition
+            } else if (uintReturndata == 1) {


Suggested change

} else if (uintReturndata == 1) {

}

// true condition

else if (uintReturndata == 1) {

nintynick · 2023-02-22T03:37:37Z

src/Hats.sol

+        /* 
+        if function call succeeds with data of length == 32, then we know the contract exists 
+        and has the getHatStatus function.
+        But we still can't assume that the return data is a boolean, so we need to check that manuallys


Suggested change

But we still can't assume that the return data is a boolean, so we need to check that manuallys

But we still can't assume that the return data is a boolean, so we need to check that manually

nintynick · 2023-02-22T03:40:42Z

src/HatsIdUtilities.sol

@@ -133,11 +162,11 @@ contract HatsIdUtilities is IHatsIdUtilities {
    /// @return uint256 The hat id of the resulting admin
    function getAdminAtLevel(uint256 _hatId, uint32 _level) public view returns (uint256) {
        uint256 linkedTreeAdmin = linkedTreeAdmins[getTophatDomain(_hatId)];


perhaps just linkedAdmin?

nintynick · 2023-02-22T03:44:35Z

src/HatsIdUtilities.sol

+        uint32 localTopHatLevel = getHatLevel(getLocalAdminAtLevel(_hatId, 0));

-        if (localTopHatLevel <= _level) return getTreeAdminAtLevel(_hatId, _level - localTopHatLevel);
+        if (localTopHatLevel <= _level) return getLocalAdminAtLevel(_hatId, _level - localTopHatLevel);


spengrah added 6 commits February 20, 2023 11:16

TRST-H-1: add static balance check to mintHat

7293e60

TRST-M-7: check static balance on renounceHat

7223e27

TRST-L-1: uint16(_admin) check on createHat

4c27a04

TRST-L-2: fix tophat imageURI fallback

5cde1ad

TRST-L-5 revert on zero-address toggle & eligibility

13dd59f

address other minor Trust recs

62c04a0

spengrah requested a review from topocount February 20, 2023 20:15

spengrah added 9 commits February 20, 2023 14:19

address remaining minor Trust recs

8d6b723

TRST-L-3 protect against invalid return data (dirty impl.)

c580e1c

rebase from mitigation/trust-audit

f83f521

TRST-L-4 refactor isAdminOfHat (and related functions

cebbc01

TRST-M-4 add eligibility check to mintHat and transferHat

5ee3f49

TRST-M-1 clarify that Hats is ERC1155-similar

1118c27

TRST-L-3 clean up toggle and eligibility returndata handling

d2a0776

fix readme

aff1716

clean up logs

2cfad31

spengrah marked this pull request as ready for review February 21, 2023 23:04

topocount approved these changes Feb 22, 2023

View reviewed changes

nintynick requested changes Feb 22, 2023

View reviewed changes

address @nintynick and @topocount review suggestions

1d62512

spengrah merged commit c5bfd82 into mitigation/trust-audit Feb 22, 2023

spengrah deleted the mitigation/trust-audit-spencer branch February 22, 2023 04:28


		Hats Protocol conforms fully to the ERC1155 interface. All external functions required by the [ERC1155 standard](https://eips.ethereum.org/EIPS/eip-1155) are exposed by Hats Protocol. This is how Hats can work out of the box with existing token-gating applications.

		However, Hats Protocol is not fully compliant with the ERC1155 standard. Since Hats are not transferable by their owners (aka "wearers"), there is little need for safe transfers and the `ERC1155TokenReceiver` logic. Developers building on top of Hats Protocol should note that mints and transfers of Hats will not, for example, include calls to `onERC1155Received`.

	\| Signer on a multisig \| Using the Hat's ERC1155--similar token as a condition for membership in an Orca Protocol Pod \|
	\| Signer on a multisig \| Using the Hat's ERC1155-similar token as a condition for membership in an Orca Protocol Pod \|

	/// @dev This is a multitenant contract that can manage all hats for a given chain. While it fully implements the ERC1155 interface, it does not comply full with the ERC1155 standard.
	/// @dev This is a multitenant contract that can manage all hats for a given chain. While it fully implements the ERC1155 interface, it does not fully comply with the ERC1155 standard.

	But we still can't assume that the return data is a boolean, so we need to check that manuallys
	But we still can't assume that the return data is a boolean, so we need to check that manually

Conversation

spengrah commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topocount left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topocount Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nintynick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spengrah commented Feb 20, 2023 •

edited

Loading

topocount Feb 22, 2023 •

edited

Loading