search

Hats-Protocol / hats-protocol

Core contracts for Hats Protocol v1.0
https://www.hatsprotocol.xyz
GNU Affero General Public License v3.0
83 stars 25 forks source link

2 - cap size of details and imageURI #114

Closed spengrah closed 1 year ago

spengrah commented 1 year ago

https://github.com/sherlock-audit/2023-02-hats-judging/issues/2

zobront commented 1 year ago

This PR appears to cap the length in the change... functions but still allows it to be set higher when the hat is originally created. Does it make sense to add a similar check in _createHat()?

zobront commented 1 year ago

Same thing with the _linkTopHatToTree() function, I believe.

spengrah commented 1 year ago

This PR appears to cap the length in the change... functions but still allows it to be set higher when the hat is originally created. Does it make sense to add a similar check in _createHat()?

@zobront the thinking here is that we don't need the check on creation since — due to writing being more expensive than reading — it will be impossible to write a single string that can DOS reads. Only by incrementally updating can a DOS be achieved, hence the choice to apply at the changeHat...() level. This also gives initial hat creators some additional flexibility if they desire, and (more importantly) minimizes cost for a common function.

Let me know if you disagree with this analysis.

Same thing with the _linkTopHatToTree() function, I believe.

Ah, good catch. Since this can be called from within relink...() multiple times, its subject to the iterative write attack, so it should have this check.

zobront commented 1 year ago

@spengrah Great, agreed with this reasoning and the fix in #118 looks perfect.