search

makerdao / dss-proxy

GNU Affero General Public License v3.0
21 stars 6 forks source link

Remove onlyOnwer and just use auth #5

Closed talbaneth closed 2 years ago

talbaneth commented 2 years ago

We might want to try and save these additional 2 storage reads (from same slot) As I understand the reason to have such check might be:

  1. To make sure that only the owner (and not the authority) can change the owner (through setOwner).
  2. To make sure we have an event emitted on owner change.

However we do not have any similar check that authority hasn't changed during the execution, and malicious authority can do the same harm as malicious owner. So (1) is not that useful (assuming we don't want to check authority as well and add 2 more reads). Regarding (2), not sure if the event tracking really justifies another read. We did manage without that with the old ds-proxy.

If we do go along with this change than there is still a difference from the old ds-proxy (which used ds-auth), which is that setOwner and setAuthority will only be callable by the owner (and not the authority). That is probably fine, though might be good to verify with UIs (don't think it should block sending to audit).

talbaneth commented 2 years ago

@gbalabasquer lastest commits look good, feel free to merge.