Old and malicious wards can take over other wards privileges

Summary

Old wards (authorized addresses) retain their privileges across contract upgrades and can remove all new or old wards.

Vulnerability Detail

The current implementation of the wards system does not reset or update ward status during upgrades. This means that all previously authorized addresses retain their privileges, and they can remove new wards added during an upgrade.

Impact

This creates a potential centralization risk and could lead to unexpected behavior in the authorization system after upgrades. It may allow outdated or compromised ward addresses to maintain control over the contract.

Code Snippet

Nst.sol

function initialize() initializer external {
    __UUPSUpgradeable_init();
    wards[msg.sender] = 1;
    emit Rely(msg.sender);
}

function deny(address usr) external auth {
    wards[usr] = 0;
    emit Deny(usr);
}

Tool used

Manual Review

Recommendation

Implement a more robust authorization system that is upgrade-aware. Consider using OpenZeppelin's AccessControlUpgradeable or create a custom system that allows for resetting or updating ward status during upgrades. For example:

function _authorizeUpgrade(address newImplementation) internal override auth {
    // Reset all wards
    // Add new wards or transfer existing ones as needed
}

A timelock can also be helpful.

Duplicate of #48

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

Deivitto - Old and malicious wards can take over other wards privileges #124