sherlock-audit / 2024-10-ethos-network-judging

0 stars 0 forks source link

PNS - Corruptible Upgradability Pattern #268

Open sherlock-admin2 opened 2 weeks ago

sherlock-admin2 commented 2 weeks ago

PNS

Medium

Corruptible Upgradability Pattern

Summary

Storage of EthosAttestation.sol, EthosDiscussion.sol, EthosProfile.sol, EthosReview.sol, EthosVote.sol might be corrupted during an upgrade.

EthosProfile.sol

Root Cause

The project uses OpenZeppelin version 5, which incorporates a structured storage schema. In this model, each base contract defines a dedicated storage slot for its structure containing all variables necessary for its operations. The project also implements custom versions of several base classes to extend functionality according to its requirements. However, these custom base contracts do not use the structured storage pattern; instead, they define separate variables at the contract level.

The project’s inheritance hierarchy is complex and extensively leverages multiple inheritance rather than a simple linear structure. This, combined with a reliance on proxy contracts for deployment, means that updating base contracts will be challenging, if not impossible. Further complicating this is the mixture of non-upgradable and upgradable base contracts from OpenZeppelin; some are from the standard OpenZeppelin library and define their own contract-level variables without supporting upgradability.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

These factors create a fragmented upgradeability model, which may lead to significant issues when performing contract upgrades. Updating underlying contracts may lead to inconsistencies in the data stored in the proxy contract storage.

PoC

Below is the inheritance diagram for EthosProfile , other contracts inherit analogously.

graph BT;
    classDef nostruct fill:#f96;
    classDef struct fill:#99cc00;
    EthosProfile:::nostruct-->AccessControl:::nostruct
    EthosProfile:::nostruct-->UUPSUpgradeable::::struct
    AccessControl:::nostruct-->Pausable
    AccessControl:::nostruct-->AccessControlEnumerable:::classic
    AccessControl:::nostruct-->SignatureControl:::nostruct
    SignatureControl:::nostruct-->Initializable

Mitigation

To stabilize the upgradability pattern: