Axelar Network contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L484 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L490 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L529

Vulnerability details

Impact

Since this is important, we quote it again instead of referring to our other bug report on a different, yet related bug. The context within which a command is executed is extremely important.

AxelarGatewayMultisig.execute() takes a signed batch of commands. Each command has a corresponding commandID. This is guaranteed to be unique from the Axelar network. execute intentionally allows retrying a commandID if the command failed to be processed; this is because commands are state dependent, and someone might submit command 2 before command 1 causing it to fail.

Thus if an attacker manages to rearrange execution order of commands within a batch, it should probably be treated seriously. This is exactly what might happen here due to reentrancy. A malicious player that managed to gain reentrancy over execute can easily execute later commands in a batch before earlier commands are fully executed, effectively breaking all assumptions on command executed context.

Proof of Concept

The _execute function and its wrapper execute are both reentrant.

    function execute(bytes calldata input) external override;
    function _execute(bytes memory data, bytes[] memory signatures) internal;

Thus if an attacker manages to reenter the _execute function with the same batch of commands and signatures, previously successfully executed and ongoing commands will be skipped due to premature marking of the success flag.

        if (isCommandExecuted(commandId)) continue; /* Ignore if duplicate commandId received */

This allows later commands to be executed before the current ongoing command is finished. The reentrant attack can be nested to perform further reordering of commands.

Generally speaking, other unrelated batches of signed commands can only be executed, but since the assumption of ordering is most likely stronger within a single batch, we focus on illustrating the single batch scenario above.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Make execute nonReentrant

Add an ever increasing nonce to signatures to prevent replay

    function execute(bytes calldata input) nonReentrant external override {
        ...
    }

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Summary

We list 2 low-critical findings and 4 non-critical findings here:

• (Low) No checks against provided symbol and actual symbol
• (Low) Implement upgrade() function in proxy rather than implementation
• (Non) ECDSA only handles specific types of signatures
• (Non) ERC20Permit timestamp<deadline instead of timestamp<=deadline (different from original implementation)
• (Non) It’s better to follow common upgradeable patterns
• (Non) Use EIP712 instead of encodeObjects

In conclusion, it's better to check the token symbol, use original implementation of ECDSA, and follow EIP712.

(Low) No checks against provided symbol and actual symbol

Impact

_deployToken function in AxelarGateway.sol doesn't check against the provided symbol and actual symbol of external tokens.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGateway.sol#L336

Recommended Mitigation Steps

Check param symbol to equal the actual symbol of external token:

        require(symbol == token.symbol(), "Symbol doesn't match");

(Low) Implement upgrade() function in proxy rather than implementation

Impact

The upgrade() function is implemented in AxelarGateway. If the new AxelarGateway implementation doesn't have an upgrade() function, the proxy will not be upgradable.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGateway.sol#L258

Recommended Mitigation Steps

Move upgrade() to the proxy contract.

(Non) ECDSA only handles specific types of signatures

Impact

In ECDSA.sol, recover()only handles specific types of signatures. But it should be able to take care of more types.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/ECDSA.sol#L31

Recommended Mitigation Steps

Follow the original implementation of ECDSA.

(Non) ERC20Permit timestamp<deadline instead of timestamp<=deadline (different from original implementation)

Impact

In the original openzeppelin implementation, ERC20Permit/permit() uses block.timestamp <= deadline, but in Axelar, it uses block.timestamp < deadline. While this is harmless, we recommend developers to conform to the publicly agreed standards while reimplementing contracts.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/ERC20Permit.sol#L43

Recommended Mitigation Steps

Use require(block.timestamp <= deadline, 'EXPIRED'); instead

(Non) It’s better to follow common upgradeable patterns

Impact

The implementation of upgradeable contracts seems to be fine, but it is better to follow common upgradeable patterns, like using initialize instead of setup. And guard initialization functions with the initializer modifier instead of adding a dummy function in Proxy to prevent function delegation.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGateway.sol#L66

Recommended Mitigation Steps

Follow common upgradeable patterns

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

(Non) Use EIP712 instead of encodeObjects

Impact

Current implementations require extreme careful choice between encode and encodePacked, since using encodePacked on data including multiple dynamic length fields will most likely make injections possible. We have manually reviewed and confirmed all choices present in contract are correct, but still advise developers to adopt EIP712 for robustness.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGateway.sol#L482

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGateway.sol#L506

Recommended Mitigation Steps

Adopt EIP712

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L118 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L140 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L181 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L271 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L293 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L332 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L332 https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L526

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Calculate toEthSignedMessageHash once and cache

In the _execute function, it calculates toEthSignedMessageHash many times in loop

Proof of Concept

https://github.com/code-423n4/2022-04-axelar/blob/main/src/AxelarGatewayMultisig.sol#L496

Recommendation

Calculate toEthSignedMessageHash once and cache to save gas:

        bytes32 hash = ECDSA.toEthSignedMessageHash(keccak256(data));
        for (uint256 i; i < signatureCount; i++) {
            signers[i] = ECDSA.recover(hash, signatures[i]);
        }