hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 1 forks source link

Some palmera functionalities could be totally DoS-ed #18

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x5eef71904120d4f553dbf649a2f938bd7fd4f0dc841bb54613b3137c955fcb13 Severity: high

Description: Description\ Because of the way palmera iterates over all indexId, which is a variable indefinitely increasing, functions which are using it to check/fetch things could be DoS with OOG reverts. One such function is the Palmera Guard, when caller is lead:

        } else {
            if (!palmeraModule.isSafe(caller)) {
                bool isSafeLead;
                // Caller is EAO (lead) : check if it has the rights over the target safe
                for (uint256 i = 1; i < palmeraModule.indexId();) {
                    if (palmeraModule.isSafeLead(i, caller)) {
                        isSafeLead = true;
                        break;
                    }
                    unchecked {
                        ++i;
                    }
                }
                if (!isSafeLead) {
                    revert Errors.NotAuthorizedAsNotSafeLead();
                }
            }
        }

You can see that there is a for loop until palmeraModule.indexId(). Now we can check that anybody can indefinitely increase this var by creating new organizations with random names:

    function createRootSafe(address newRootSafe, string calldata name)
        external
        IsSafe(newRootSafe)
        IsRootSafe(_msgSender())
        requiresAuth
        returns (uint256 safeId)
    {
...
        uint256 newIndex = indexId;
        safeId = _createOrgOrRoot(name, caller, newRootSafe);
...

        emit Events.RootSafeCreated(org, newIndex, caller, newRootSafe, name);
    }
function _createOrgOrRoot(
    string memory name,
    address caller,
    address newRootSafe
) private returns (uint256 safeId) {

... safeId = indexId++; safes[org][safeId] = DataTypes.Safe({ tier: DataTypes.Tier.ROOT, name: name, lead: address(0), safe: newRootSafe, child: new uint256, superSafe: 0 }); indexSafe[org].push(safeId); ... } For loop may become so big that the block gas limit is hit. This is a major issue because the functionality is bricked. If more places, where the following is a problem are found, It will be posted in the comments.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

NicolaMirchev commented 1 week ago

Additionally orgHash array could be infinitely large and all functions, which are using getOrgHashBySafe DoS-ed, or made super expensive:

    function getOrgHashBySafe(address safe) public view returns (bytes32) {
        for (uint256 i; i < orgHash.length;) {
            if (getSafeIdBySafe(orgHash[i], safe) != 0) {
                return orgHash[i];
            }
            unchecked {
                ++i;
            }
        }
        return bytes32(0);
    }
0xRizwan commented 1 week ago

duplicate of #10