setContracts() can be called multiple times allowing owner to set addresses more than once

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/ExchangeConfig.sol#L51

Vulnerability details

Impact

Contract ExchangeConfig.sol states that the following variables can be set only once:

    IDAO public dao; // can only be set once
    IUpkeep public upkeep; // can only be set once
    IInitialDistribution public initialDistribution; // can only be set once
    IAirdrop public airdrop; // can only be set once

    // Gradually distribute SALT to the teamWallet and DAO over 10 years
    VestingWallet public teamVestingWallet;     // can only be set once
    VestingWallet public daoVestingWallet;      // can only be set once

It enforces this by the following check inside setContracts():

        // setContracts is only called once (on deployment)
    require( address(dao) == address(0), "setContracts can only be called once" );

However, this check is not sufficient. The owner can -

1. Make the first call to setContracts() with _dao address as address(0). He can set the other addresses to some initial value. For example, set upkeep to address(0x2).
2. He can then later call setContracts() again. Since dao address is still zero, the require statement will pass and he can choose to reassign upkeep (& other variables) to address(0x99).

This bypasses the intended functionality.

Proof of Concept

Add the following inside src/root_tests/ExchangeConfig.t.sol and run with COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.sepolia.org/ --mt test_SetContractsCalledMoreThanOnce:

    function test_SetContractsCalledMoreThanOnce() public {
        // Arrange: Deploy the contract and setup contracts for the first time
        vm.prank(address(this));
        ExchangeConfig exchangeConfig = new ExchangeConfig(salt, wbtc, weth, dai, usds, managedTeamWallet);

        IDAO mockDao = IDAO(address(0));
        IUpkeep mockUpkeep = IUpkeep(address(0x2));
        IInitialDistribution mockInitialDistribution = IInitialDistribution(address(0x3));
        IAirdrop mockAirdrop = IAirdrop(address(0x4));

        // @audit : Call setContracts for the first time with `dao` as address(0)
        exchangeConfig.setContracts(mockDao, mockUpkeep, mockInitialDistribution, mockAirdrop, teamVestingWallet, daoVestingWallet);

        // @audit : Calling setContracts again does not revert and allows changing the `upkeep` address to a new one
        exchangeConfig.setContracts(mockDao, IUpkeep(address(0x99)), mockInitialDistribution, mockAirdrop, teamVestingWallet, daoVestingWallet);
    }

Tools used

Foundry

Recommended Mitigation Steps

Apply the following patch to add a boolean variable which keeps track of the first call of setContracts():

diff --git a/src/ExchangeConfig.sol b/src/ExchangeConfig.sol
index ca63f13..050e023 100644
--- a/src/ExchangeConfig.sol
+++ b/src/ExchangeConfig.sol
@@ -32,6 +32,8 @@ contract ExchangeConfig is IExchangeConfig, Ownable

    IAccessManager public accessManager;

+   bool isAlreadySet;
+

    constructor( ISalt _salt, IERC20 _wbtc, IERC20 _weth, IERC20 _dai, IUSDS _usds, IManagedWallet _managedTeamWallet )
        {
@@ -48,7 +50,8 @@ contract ExchangeConfig is IExchangeConfig, Ownable
    function setContracts( IDAO _dao, IUpkeep _upkeep, IInitialDistribution _initialDistribution, IAirdrop _airdrop, VestingWallet _teamVestingWallet, VestingWallet _daoVestingWallet ) external onlyOwner
        {
        // setContracts is only called once (on deployment)
-       require( address(dao) == address(0), "setContracts can only be called once" );
+       require( !isAlreadySet, "setContracts can only be called once" );
+       isAlreadySet = true;

        dao = _dao;
        upkeep = _upkeep;

Also, in this context, it would be better to add the explicit check that dao is not being set to address(0) by mistake.

Assessed type

Invalid Validation

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)