bin2chen - changeTreasury() Lack of check and remove old

[sherlock-admin]

bin2chen

medium

changeTreasury() Lack of check and remove old

Summary

changeTreasury() Lack of check and remove old

Vulnerability Detail

changeTreasury() used to set new treasury
The code is as follows：

    function changeTreasury(uint256 _marketId, address _treasury)
        public
        onlyTimeLocker
    {
        if (_treasury == address(0)) revert AddressZero();

        address[2] memory vaults = marketIdToVaults[_marketId];

        if (vaults[0] == address(0) || vaults[1] == address(0)) {
            revert MarketDoesNotExist(_marketId);
        }
        IVaultV2(vaults[0]).whiteListAddress(_treasury);
        IVaultV2(vaults[1]).whiteListAddress(_treasury);
        IVaultV2(vaults[0]).setTreasury(treasury);
        IVaultV2(vaults[1]).setTreasury(treasury);

        emit AddressWhitelisted(_treasury, _marketId);
    }

The above code has the following problem:

1. no check whether the new treasury same as the old. If it is the same, the whitelist will be canceled.
2. Use setTreasury(VaultFactoryV2.treasury), it should be setTreasury(_treasury)
3. not cancel old treasury from the whitelist

Impact

whiteListAddress abnormal

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultFactoryV2.sol#L228

Tool used

Manual Review

Recommendation

    function changeTreasury(uint256 _marketId, address _treasury)
        public
        onlyTimeLocker
    {
        if (_treasury == address(0)) revert AddressZero();

        address[2] memory vaults = marketIdToVaults[_marketId];

        if (vaults[0] == address(0) || vaults[1] == address(0)) {
            revert MarketDoesNotExist(_marketId);
        }

+       require(vaults[0].treasury() !=_treasury,"same"); //check same
+       IVaultV2(vaults[0]).whiteListAddress(vaults[0].treasury()); //cancel old whitelist
+       IVaultV2(vaults[1]).whiteListAddress(vaults[1].treasury()); //cancel old whitelist

        IVaultV2(vaults[0]).whiteListAddress(_treasury);
        IVaultV2(vaults[1]).whiteListAddress(_treasury);
+       IVaultV2(vaults[0]).setTreasury(_treasury);
+       IVaultV2(vaults[1]).setTreasury(_treasury);
-       IVaultV2(vaults[0]).setTreasury(treasury);
-       IVaultV2(vaults[1]).setTreasury(treasury);

        emit AddressWhitelisted(_treasury, _marketId);
    }

[dmitriia]

Keeping it separate from 435 because of whitelist observation (1)

[pauliax]

Escalate for 10 USDC.

I believe it is unfair to leave it as a solo medium.

#410 also mentions the problem with whitelisting: "Also, probably the old treasury should be removed from the whitelist to prevent accidental abuse of privileges." but was grouped together with other issues from #435.

[sherlock-admin]

Escalate for 10 USDC.

I believe it is unfair to leave it as a solo medium.

#410 also mentions the problem with whitelisting: "Also, probably the old treasury should be removed from the whitelist to prevent accidental abuse of privileges." but was grouped together with other issues from #435.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[3xHarry]

fix Pr: Y2K-Finance/Earthquake#137

[hrishibhat]

Escalation accepted

Added relevant duplicates based on whitelist observation

[sherlock-admin]

Escalation accepted

Added relevant duplicates based on whitelist observation

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[IAm0x52]

Fixes look good. Carousel now directly uses the treasury address sent on factory