sherlock-audit / 2024-06-makerdao-endgame-judging

1 stars 1 forks source link

Deivitto - A compromissed ward can take over all Nst contract #126

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

Deivitto

Medium

A compromissed ward can take over all Nst contract

Summary

There is no timelock for upgrades, which can be dangerous in case of admin key compromise as it allows for immediate, potentially malicious upgrades.

Vulnerability Detail

The current implementation allows any address with ward status to upgrade the contract immediately without any delay or additional checks. This lack of a timelock mechanism creates a single point of failure if an admin key is compromised.

Impact

A compromised admin could instantly upgrade the contract to a malicious implementation, potentially breaking invariants, stealing funds, or causing other severe damage to the system and its users.

Code Snippet

Nst.sol

function _authorizeUpgrade(address newImplementation) internal override auth {}

Tool used

Manual Review

Recommendation

Implement a timelock mechanism for upgrades. This could be done by integrating with a separate timelock contract such as OZ TimelockController or by implementing a built-in delay. This approach provides a window for users to react to proposed upgrades and helps mitigate the risk of malicious immediate upgrades.

Duplicate of #48

sunbreak1211 commented 1 month ago

The ward of NST will be the PauseProxy which is a timelock. Anyway this is totally out of scope.