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

3 stars 3 forks source link

The Treasury.sol contract changes the address for the different manager contract in one function call. #417

Closed c4-bot-1 closed 9 months ago

c4-bot-1 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Treasury.sol#L156-L167

Vulnerability details

Impact

In a case where either one of the manager addresses the tokenomics, depositiory or dispenser contracts are compromised or deprecated, attempting to replace the compromised manager contract address necessitates an overall replacement of all the other addresses. This not only aggravates the gas consumption of the protocol but will also bring about unnecessary proposal being made by governance for manager addresses which do not need a replacement.

And as such when changes is made to one manager contract, changes are also required for all manager contract addresses.

Proof of Concept

In a situation whereby either of tokenomics, depository or dispenser contract becomes deprecated or compromised or updates needs to made due to issues in logic, changes need to be made for all contract addresess.

For example, If the Tokenomics contract address needs to updated but the dispenser and depository contracts needs no update. Update to the Tokenomics contract will require additional updates to both the dispenser and depository contract address.

  1. This will require additional proposals being made for the replacement of the two other contracts which isn't needed from the onset.
  2. This can also lead to wrong contract addresses being assigned to the other manager address that were not originally intended to be inputted by the owner of the contract.
  function changeManagers(address _tokenomics, address _depository, address _dispenser) external {
        // Check for the contract ownership
        if (msg.sender != owner) {
            revert OwnerOnly(msg.sender, owner);
        }

        // Change Tokenomics contract address
        if (_tokenomics != address(0)) {
            tokenomics = _tokenomics;
            emit TokenomicsUpdated(_tokenomics);
        }
        // Change Depository contract address
        if (_depository != address(0)) {
            depository = _depository;
            emit DepositoryUpdated(_depository);
        }
        // Change Dispenser contract address
        if (_dispenser != address(0)) {
            dispenser = _dispenser;
            emit DispenserUpdated(_dispenser);
        }
    }

Tools Used

Manual review, VSCODE

Recommended Mitigation Steps

Different functions should be made available for the replacement of each manager addresses instead of just one function being used to replace all three managers all at once.

Assessed type

Other

c4-pre-sort commented 9 months ago

alex-ppg marked the issue as duplicate of #433

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: Invalid