singnet / snet-upgradeable-owners-minting-policy

0 stars 1 forks source link

UpgradeableOwners script - Audit #7

Open zmrocze opened 2 weeks ago

zmrocze commented 2 weeks ago

UpgradeableOwners script

User Requirements

✅ A user can mint the protocol token if the owners agree.

This is achievable spending the protocol UTXO with the TokenMint redeemer. The UpgradeableOwners validator which owns the UTXO verifies the provided signatures. That is the validator ensures that there is at least the specified threshold many signatures from distinct owners signing the transaction. A user can prepare the minting transaction and send it sequentially to owners to gather their signatures.

✅ A user can add or remove protocol owners and change the minimum threshold if the owners agree.

Adding an owner is achievable by spending the protocol UTXO with AddOwner redeemer. The validator verifies that the new datum contains a new owner prepended to the previous list of owners. The new threshold is verified correctly against an updated number of owners.

Removing an owner is achievable by spending the protocol UTXO with RemoveOwner redeemer. The validator verifies that the owner list in the new datum has no occurences of the to be removed owner.

The validator should check the new threshold against an updated number of owners. Instead, the validator checks that the new threshold is no greater than n-1 where n is the previous length of owners list. This check is incorrect when the owner to be removed was not in fact an owner!

The result of that is that the threshold check is more strict than it needs to be when the to be removed PubkeyHash is not an owner. Arguably, such a transaction has no purpose and the check being more strict than necessary doesn't lead to any exploits. Therefore, the validator's code doesn't need corrections, but the caveat is to be aware of.

The validator should additionally to the existing checks also check that the new owner is not already present in the owners list. We discuss that in the vulnerabilities section.

✅ A user can set new threshold number and change the set of owners in single transaction.

Yes. In fact a new threshold has to be specified by the Redeemer but it can possibly be equal to the previous threshold. The validator correctly verifies that the threshold is within bounds, with the caveat described in the requirement TODO-NUMBER.

Protocol Requirements

❌ Owners agree to do a certain protocol action if the transaction is signed by at least the threshold number of pubkeys associated with the addresses in the owners list recorded in the datum.

The UpgradeableOwners validator owning the protocol UTXO validates the provided transaction signatories against the specification contained in the UTXO datum. Firstly, it checks the new datum in the continuing output:

The new threshold is correctly checked against an updated number of owners, with the caveat described in the requirement TODO-NUMBER.

Secondly, it checks for presence of the required number of owners' signatures. The check counts how many owner PubkeyHashes appear in the transaction signatories. The check is insufficient. For the check to be correct we cannot allow duplicates in the owners list. Allowing duplicates leads to this and other vulnerabities described in greater detail in TODO-link.

✅ The minimum threshold is no greater than the number of owners and greater than 1.

This is ensured by the validator that explicitly checks if the new threshold is greater than 1 and no greater than the current number of owners. The current number of owners is not calculated correctly in the malformed case of a transaction that removes an owner whos not present in the owners list. In that case though the check is only more strict than necessary, not leading to any vulnerabilities.

❌ It is impossible to stall the protocol by creating an unspendable output.

It is possible to stall the protocol by locking unspendable tokens at the protocol UTXO. The vulnerability follows a common pattern of Foreign tokens vulnerability and is described in more detail in the TODO-sectionlink.

❌ The list of owners must not have duplicates.

TODO.

This was not the case. The following pr fixed it in this way.

✅ The token minting transaction doesn’t change owners and threshold.

Yes, the validator checks explicitly that the continuing output contains an unchanged datum.

❌ Protocol allows the minting of only a single asset class.

That's not correct. The Token script is parametrized with a token name - and in fact for the given parametrized Token policy there's a single allowed token name for minted tokens. Note though, that no concrete parametrization of the token policy is expected by other validators of the protocol. Concretely, both the UpgradeableOwners and Token are bound to work with the single state thread NFT. After a correct initialization, it is ensured that the NFT token is held at an UpgradeableOwners script address, therefore the verification delegation from Token script to UpgradeableOwners validator by checking for NFT presence is justified. The issue appears once we see that the validator does not depend on the Token script - on its hash or any concrete parametrization. This means that, provided that the owners agree, the protocol can mint tokens from various asset classes using the TokenMint redeemer and parametrizing the Token policy with different token names.

Timekiller7 commented 2 weeks ago

RegardingProtocol allows the minting of only a single asset class. section.
That wasn't a requirement in our case. Why we can't use one UpgradeableOwners script in order to mint other tokens? Could you explain, please, what are the vulnerabilities in this approach?