Inconsistent permission levels to change tssAddress might cause DOS of certain crosschain tx

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/ZetaConnector.base.sol#L129

Vulnerability details

Impact

ERC20Custody.sol and ZetaConnector.eth.sol (ZetaConnector.non-eth.sol) might have different stored tssAddress at the same point, which might cause DOS for certain crosschain tx such as ERC20 crosschain deposit or zetatoken deposit, or if the crosschain tx is reverted message, then the reverted tx can be aborted causing loss of user refunds.

Proof of Concept

In ERC20Custody.sol, the permission level for update tssAddress is set for TSSAddressUpdater, which is intended to be a multi-sig contract -Threshold Signature Scheme (TSS) [GG20] is a multi-sig ECDSA/EdDSA protocol(based on code doc). This multi-sig is intended to have the authority to change tssAddress in case of Zeta blockchain validator nodes churn. When this multi-sig renounce the TSSAddressUpdater role, only then will TSSAddress(collectively possessed by Zeta validators) will assume the TSSAddressUpdater role. This means TSSAddress cannot update its own address until the multi-sig renounces its TSSAddressUpdater role.

However, in ZetaConnector.base.sol, the permission level for updating tssAddress is set differently. At the beginning, both the multi-sig contract and tssAddress can update tssAddress. This means it doesn't matter whether the mult-sig contract renounce ownership or not, tssAddress can always change its own address.

This is problematic when tssAddress is updated on ZetaConnector.base.sol by tssAddress before the multi-sig renounces ownership on ERC20Custody.sol, because the tssAddress in storage in ZetaConnector.base.sol, and the stored address in ERC20Custody.sol are different. Sensitive crosschain functions will have permissions set to different tssAddress, causing unexpected reverts which may cause DOS.

//contracts/evm/ZetaConnector.base.sol
    function updateTssAddress(address tssAddress_) external {
        //@audit this allows both tssAddress and multi-sig to update tssAddress
|>      if (msg.sender != tssAddress && msg.sender != tssAddressUpdater) revert CallerIsNotTssOrUpdater(msg.sender);
        if (tssAddress_ == address(0)) revert ZetaCommonErrors.InvalidAddress();
        tssAddress = tssAddress_;
...

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/ZetaConnector.base.sol#L129)

//contracts/evm/ERC20Custody.sol
    //@audit only TSSAddressUpdater (originally multi-sig contract) can change tssAddress
    function updateTSSAddress(address TSSAddress_) external onlyTSSUpdater {
        if (TSSAddress_ == address(0)) {
            revert ZeroAddress();
        }
        TSSAddress = TSSAddress_;
        emit UpdatedTSSAddress(TSSAddress_);
    }
    modifier onlyTSSUpdater() {
|>      if (msg.sender != TSSAddressUpdater) {
            revert InvalidTSSUpdater();
        }
...

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/ERC20Custody.sol#L80)

Suppose at one point ERC20Custody.sol stores an outdated tssAddress while ZetaConnector.base has the active tssAddress. In ERC20Custody.sol, the onlyTSS controlled withdraw() will be reverted due to a failed onlyTSS check. This will impact the user flow of withdrawing deposited ZRC20 token. The withdrawal is initiated on zetaChain ZRC20.sol, and crosschain tx will be created but the observation will fail due to revert on ERC20Custoday.sol.

Tools Used

Manual review

Recommended Mitigation Steps

In ZetaConnector.base.sol updateTssAddress(), change the if statement into if (msg.sender != tssAddressUpdater) revert CallerIsNotTssOrUpdater(msg.sender), which will align the permission level in both contracts.

Assessed type

Other

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #127

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality