code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L173-L183 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L156-L161

Vulnerability details

Impact

In CrossChainCanonicalBase.sol, recoverERC20() is supposed to be called by the owner. And it seems like that the owner cannot take enabled bridgeToken. However, a malicious owner can easily break the rule and take any enabled bridgeToken unconditionally.

Proof of Concept

It seems like that the owner cannot take bridgeToken when bridgeTokenEnabled[tokenAddress] is true https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L173-L183

    function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyOwner {
        if (tokenAddress == address(this)) {
            revert IllegalArgument();
        }

        if (bridgeTokenEnabled[tokenAddress]) {
            revert IllegalState();
        }

        TokenUtils.safeTransfer(address(tokenAddress), msg.sender, tokenAmount);
    }

However, a malicious owner can easily break the rule by calling toggleBridgeToken() https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L156-L161

    function toggleBridgeToken(address bridgeTokenAddress, bool enabled) external onlyOwner {
        // Toggle swapping
        bridgeTokenEnabled[bridgeTokenAddress] = enabled;

        emit BridgeTokenToggled(bridgeTokenAddress, enabled);
    }

Tools Used

None

Recommended Mitigation Steps

Try to add some limitations on the owner.

0xfoobar commented 2 years ago

Sponsor acknowledged

Assumed that governance acts in good faith.

0xleastwood commented 2 years ago

Typically this would be in-line with other governance related issues which have been downgraded to QA, however, I believe this finding is unique and justified as it outlines a clear path for how the protocol's governance could drain the protocol of all its bridge tokens. I'd like to hear your thoughts @0xfoobar as its possible this has the same impact as other issues which are dependent on a malicious governance.

0xfoobar commented 2 years ago

This is a potential path for governance to extract bridge tokens from the contract. Don't see it as a concern to be elevated above other governance control vectors, as governance can also add arbitrary new bridge tokens, etc.