search

code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

Single-step change of governance address is extremely risky #44

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

Single-step change of critical governance address and lack of zero address check is extremely risky. If a zero address or incorrect address (private key not available) is used accidentally, or maliciously changed by a compromised governance account then the entire governance of the protocol is locked forever or lost to an attacker. No governance changes can be made by authorized governance account and protocol will have to be redeployed. The reputation of the protocol will take a huge hit. There may be significant fund lock/loss as well.

Interestingly, this 2-step process is applied to the changing of Strategist address but not Governance address. Governance has more authority in the protocol because it can change the Strategist among other things. So this 2-step should definitely be applied to Governance as well.

Given the magnitude of the impact, i.e. permanent lock of all governance actions, potential lock/loss of funds, and the known/documented failures of wallet opsec, this risk is classified as medium severity.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/Manager.sol#L223-L236

Strategist: https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/Manager.sol#L333-L345 https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/Manager.sol#L402-L413

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change of the most critical protocol address i.e. governance should be timelocked and be a 2-step process: approve+claim in two different transactions, instead of a single-step change.

Haz077 commented 3 years ago

I only agree that there should be zero address checks, which will make this issue duplicated.

GalloDaSballo commented 3 years ago

Disagree with finding:

  1. Allowing to set to address(0) allows to burn admin keys, which is a good thing for a decentralized protocol
  2. While some protocols prefer a two step governance change, there's nothing inherently wrong with 1-step Downgrading to non-critical