c4-bot-7 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Tokenomics.sol#L384

Vulnerability details

Impact

Unsafe upgrade can break protocol

Analysis of the vulnerability

You are using a custom upgrade for the tokenomics contract (not following UUPS standard)

 function changeTokenomicsImplementation(address implementation) external {
    // Check for the contract ownership
    if (msg.sender != owner) {
        revert OwnerOnly(msg.sender, owner);
    }
    // Check for the zero address
    if (implementation == address(0)) {
        revert ZeroAddress();
    }

    // Store the implementation address under the designated storage slot
    assembly {
        sstore(PROXY_TOKENOMICS, implementation)
    }
    emit TokenomicsImplementationUpdated(implementation);
}

This is a bad idea for several reasons

  1.    You missed `initializeTokenomics` inside `changeTokenomicsImplementation` that you need to update storage variables with the new variables in case you add then.
  2.   Owner is set by solidity compiler (slot 0, not complaining with EIP1967).   This could cause the update to break the owner .  Instead you should put the owner at `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` (obtained as `bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)`).  This ensures that you if you make an incorrect upgrade at least you will be able to go back.

Recommendation

Implement a complete UUPS Standard including EIP1967

Assessed type

Upgradable

c4-pre-sort commented 9 months ago

alex-ppg marked the issue as insufficient quality report

c4-judge commented 8 months ago

dmvt marked the issue as unsatisfactory: Overinflated severity

code-423n4 / 2023-12-autonolas-findings