Storage and escrow functionality can be compromised

[c4-bot-9]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Admin.sol#L126

Vulnerability details

The lack of checks or timelocks for the newImplementation addresses provided to the upgradeStorage and upgradePaymentEscrow functions can lead to an incorrect address being set. This could result in the storage or escrow functionality being compromised.

function upgradeStorage(address newImplementation) external onlyRole("ADMIN_ADMIN") {
    STORE.upgrade(newImplementation);
}

function upgradePaymentEscrow(address newImplementation) external onlyRole("ADMIN_ADMIN") {
    ESCRW.upgrade(newImplementation);
}

Impact

If an attacker gains access to an admin role or if an admin mistakenly inputs a malicious or erroneous contract address as the newImplementation, the entire system could be compromised. This could lead to loss of funds, unauthorized access to stored data, or a complete breakdown of the contract's functionality.

Mitigations

Add a check to ensure that the newImplementation address is a contract and not a regular wallet address. This can be done by checking that the code size at the address is greater than zero.

function isContract(address _addr) private view returns (bool) {
    uint32 size;
    assembly {
        size := extcodesize(_addr)
    }
    return (size > 0);
}

function upgradeStorage(address newImplementation) external onlyRole("ADMIN_ADMIN") {
    require(isContract(newImplementation), "Admin: New implementation is not a contract");
    STORE.upgrade(newImplementation);
}

function upgradePaymentEscrow(address newImplementation) external onlyRole("ADMIN_ADMIN") {
    require(isContract(newImplementation), "Admin: New implementation is not a contract");
    ESCRW.upgrade(newImplementation);
}

Time-Lock Mechanism: Implement a time-lock mechanism that delays the upgrade by a certain period (e.g., 48 hours), allowing stakeholders to review and respond to the proposed change.

Multi-Signature Approval: Require that upgrade transactions be approved by multiple signatories, ensuring that no single admin can unilaterally change critical components.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality