Redundant Redeemers

[zmrocze]

Severity	CVSS	Vulnerability Type
Low	0.0	other

Description

The protocol specifies 3 different actions for updating the protocol state, performed by submitting a transaction with AddOwner, RemoveOwner or UpdateThreshold redeemer.

All three allow an update to the threshold parameter. The first two also change the owners set. Eventhough the actions differ, the validator check on the resulting datum is essentially the same.

Recommendation

The split of protocol state modifications into these 3 actions is correct. Nevertheless, with protocol simplification in mind, a unification of the 3 actions into a single redeemer should be considered.

Possible definition for such an action could simply be:

data Redeemer
  = Update { new_owners :: [PubKeyHash], new_threshold : Integer } 
  -- | MintTokens

With this definition a care needs to be taken for the validator check to remain in a reasonable time complexity (specifically the check of new owners). An added benefit is that this representation allows to perform an update that would otherwise require multiple transactions with the previous redeemer, i.e. when adding multiple owners. The tradeof should be considered.

[zmrocze]

@Renegatto Would you agree with that? Its not any exploit, just a remark that may find someone agree with it or not

[Renegatto]

@zmrocze yeah that's pretty reasonable. MintTokens tx wouldn't even require much more CPU if continuing output checks would be made optional:

if inputStateThread == outputStateThread then True else checkStateUpdate

It would be cheap if input and output would be compared via BuiltinEq.

[Timekiller7]

@zmrocze, thank you for the feedback. Can we apply the changes now with new PR?

[zmrocze]

@Timekiller7 Hey, so the audit report you'll receive tomorrow will be based on the initial agreed commit of 8516ac1. Then in the coming time we'll make sure to use all available to us audit time to review your fixes.

I'm mentioning this because the change proposed here - and we stand by the recommendation described - is a relatively big change that could potentially introduce other problems. So my recommendation would be to wait till tomorrow and consider all the needed fixes in response together.

[Timekiller7]

@zmrocze, sure, thanks!