code-423n4 / 2024-01-renft-findings

2 stars 0 forks source link

Contract Upgrade During Execution can lead to inconsistent contract states #530

Closed c4-bot-3 closed 10 months ago

c4-bot-3 commented 10 months ago

Lines of code

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

Vulnerability details

The contract does not account for the possibility that a dependent module, such as Storage or PaymentEscrow, could be upgraded while a stopRent or stopRentBatch transaction is in progress.

function stopRent(RentalOrder calldata order) external {
    // ... existing code ...
    STORE.removeRentals(
        _deriveRentalOrderHash(order),
        _convertToStatic(rentalAssetUpdates)
    );
    // ... rest of the function ...
}

Mitigation

Implement version checks or use an upgradeable proxy pattern to ensure that the contract interacts with the correct version of the dependent modules.

// Add a version tracking for dependent modules
uint256 private _expectedStoreVersion;
uint256 private _expectedEscrowVersion;

// Use a modifier to check the version before interacting with a module
modifier checkModuleVersion(Keycode keycode, uint256 expectedVersion) {
    require(getModuleVersion(keycode) == expectedVersion, "Unexpected module version");
    _;
}

// Use the checkModuleVersion modifier in critical functions
function stopRent(RentalOrder calldata order) external checkModuleVersion(toKeycode("STORE"), _expectedStoreVersion) {
    // ... existing code ...
}

Impact

If a dependent module is upgraded during the execution of a transaction, it could lead to unexpected behavior or failures due to changes in the module's interface or logic. By tracking the expected versions of dependent modules and checking them before interacting, the contract ensures that it is always working with the correct version. This helps maintain the stability and predictability of the contract's behavior, even in the face of upgrades.

Assessed type

Upgradable

c4-pre-sort commented 10 months ago

141345 marked the issue as insufficient quality report

c4-judge commented 10 months ago

0xean marked the issue as unsatisfactory: Insufficient quality