Chain support cannot be removed or cleared in bridge contracts

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L121-L129 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L234-L240

Vulnerability details

Chain support cannot be removed or cleared in bridge contracts

Due to how addresses are handled and stored in the configuration settings, it is not possible to remove chain support in both source and destination bridge contracts.

Impact

The Axelar bridge service uses strings to represent addresses: messages sent to another chain need to specify its destination contract as a string. The protocol decided to follow the same representation and contract store addresses as strings as part of their configuration.

Chain support in the bridge contracts is then represented by associating the chain name with the address of the contract, stored as a string. This can be seen in the implementation of setDestinationChainContractAddress() for the SourceBridge contract, which stores the string, and the implementation of addChainSupport() in the DestinationBridge contract, which stores the hash of the string:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L121-L129

121:   function setDestinationChainContractAddress(
122:     string memory destinationChain,
123:     address contractAddress
124:   ) external onlyOwner {
125:     destChainToContractAddr[destinationChain] = AddressToString.toString(
126:       contractAddress
127:     );
128:     emit DestinationChainContractAddressSet(destinationChain, contractAddress);
129:   }

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L234-L240

234:   function addChainSupport(
235:     string calldata srcChain,
236:     string calldata srcContractAddress
237:   ) external onlyOwner {
238:     chainToApprovedSender[srcChain] = keccak256(abi.encode(srcContractAddress));
239:     emit ChainIdSupported(srcChain, srcContractAddress);
240:   }

This also means that checks need to be done based on the stored representation. SourceBridge checks the length of the string, while DestinationBridge checks for a bytes32(0) value:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L68-L71

68:     if (bytes(destContract).length == 0) {
69:       revert DestinationNotSupported();
70:     }
71: 

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L96-L98

96:     if (chainToApprovedSender[srcChain] == bytes32(0)) {
97:       revert ChainNotSupported();
98:     }

Note that this implies that there is no way to clear these settings and remove chain support. In the case of SourceBridge, any address sent to setDestinationChainContractAddress() will be converted to their string representation which will always have length greater than zero. For the DestinationBridge, addChainSupport() will hash the address parameter and it will be impossible for that hash value to be zero (since it will imply knowing the preimage of zero).

Proof of Concept

1. Admin configs destination address in SourceBridge by calling setDestinationChainContractAddress("optimism", destinationAddress).
2. Admin decides to remove support for Optimism.
3. Admin calls setDestinationChainContractAddress("optimism", address(0)), but this will actually store the string for the zero address "0x0000....000".
4. The check bytes(destContract).length == 0 will fail and messages will still be routed.

Recommended Mitigation Steps

Provide functions in both contracts to allow the owner to clear the settings by resetting their configuration to the default value.

function removeDestinationChainContractAddress(
  string memory destinationChain
) external onlyOwner {
  delete destChainToContractAddr[destinationChain];
  emit DestinationChainContractAddressRemoved(destinationChain);
}

function removeChainSupport(
  string calldata srcChain
) external onlyOwner {
  delete chainToApprovedSender[srcChain];
  emit ChainIdRemoved(srcChain);
}

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as duplicate of #467

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-judge]

kirk-baird marked the issue as unsatisfactory: Invalid

[c4-judge]

kirk-baird removed the grade

[c4-judge]

kirk-baird marked the issue as selected for report

[c4-judge]

kirk-baird marked the issue as satisfactory

[tom2o17]

@kirk-baird

Can I not set acceptable srcAddr to address(0) and then given address(0) cannot have a contract deployed to it, this value should not be w/n the possible ranges of srcAddr w/n the _execute function?

Granted not super ideal given it will hit a diff error msg, but functionally the result is the same.

[kirk-baird]

@kirk-baird

Can I not set acceptable srcAddr to address(0) and then given address(0) cannot have a contract deployed to it, this value should not be w/n the possible ranges of srcAddr w/n the _execute function?

Granted not super ideal given it will hit a diff error msg, but functionally the result is the same.

@tom2o17 my concern here is that there's no way to stop calling burnAndCallAxelar() which would burn tokens on the source chain. These would not be minted on the destination chain and so are essentially lost.

e.g. If you do setDestinationChainContractAddress(dstChain, address(0)) then Address.toString(address(0)) will encode that to 42 hex characters.

So this check in burnAndCallAxelar() will always pass since bytes(destContract).length is 42 bytes.

[tom2o17]

@kirk-baird

Ah I see. I am inclined to accept this issue, apologies thought this was referencing the dstBridge not the srcBridge. As mitigation we would be able to pause the contract, and prevent this functionality but agree with the auditor that this is not ideal.

[c4-sponsor]

tom2o17 (sponsor) acknowledged