Axelar Network v2 contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/AxelarGateway.sol#L262 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L98 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L110

Vulnerability details

Add cancel and refund option for Transaction Recovery

Impact

Transactions could fail or stuck, according to the documentation:

Occasionally, transactions can get "stuck" in the pipeline from a source to destination chain (e.g. due to one-off issues that arise with relayers that operate on top of the network).

Transactions have typically gotten "stuck" in the pipeline due to: (A) The transaction failing to relay from the source chain into the Axelar network for processing. (B) The transaction failing to get executed on the destination chain.

And there are several options provided:

• manual approve
• manual execute
• add gas

However, some transactions' execution depend on the time or certain condition. For example, some transaction has a deadline, it the deadline is passed, the transaction will be invalid. Or some conditions may be temporary, for example, some certain price difference for some token pair. In this case, the failed transactions will be meaningless to redo, the appropriate method is to cancel the transaction and refund. If no such option is provided, users' fund for this transaction would be lock or loss.

Proof of Concept

contracts/AxelarGateway.sol
    function approveContractCall(bytes calldata params, bytes32 commandId) external onlySelf {}
    function execute(bytes calldata input) external override {}

contracts/gas-service/AxelarGasService.sol
    function addGas() external override {}
    function addNativeGas() external payable override {}

The options are approveContractCall(), execute, addGas() and addNativeGas() are available, but no cancel and refund option.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Provide a cancel option if the transaction failed, from the source chain or destination chain, and allow the user to get the gas refund.

    function _executeWithToken(
        string memory sourceChain,
        string memory sourceAddress,
        bytes calldata payload,
        string memory tokenSymbol,
        uint256 amount
    ) internal override {
        if (price difference for some token pair > limit) {
            IERC20(token).approve(address(gateway), amount);
            gateway.sendToken(sourceChain, sourceAddress, tokenSymbol, amount);
            return;
        }
       . . .     
   }

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

contracts/interfaces/IAxelarAuthWeighted.sol
14:
    event OperatorshipTransferred(address[] newOperators, uint256[] newWeights, uint256 newThreshold);

contracts/interfaces/IAxelarGasService.sol
13-57:
    event GasPaidForContractCall(
        address indexed sourceAddress,
        string destinationChain,
        string destinationAddress,
        bytes32 indexed payloadHash,
        address gasToken,
        uint256 gasFeeAmount,
        address refundAddress
    );

    event GasPaidForContractCallWithToken(
        address indexed sourceAddress,
        string destinationChain,
        string destinationAddress,
        bytes32 indexed payloadHash,
        string symbol,
        uint256 amount,
        address gasToken,
        uint256 gasFeeAmount,
        address refundAddress
    );

    event NativeGasPaidForContractCall(
        address indexed sourceAddress,
        string destinationChain,
        string destinationAddress,
        bytes32 indexed payloadHash,
        uint256 gasFeeAmount,
        address refundAddress
    );

    event NativeGasPaidForContractCallWithToken(
        address indexed sourceAddress,
        string destinationChain,
        string destinationAddress,
        bytes32 indexed payloadHash,
        string symbol,
        uint256 amount,
        uint256 gasFeeAmount,
        address refundAddress
    );

    event GasAdded(bytes32 indexed txHash, uint256 indexed logIndex, address gasToken, uint256 gasFeeAmount, address refundAddress);

    event NativeGasAdded(bytes32 indexed txHash, uint256 indexed logIndex, uint256 gasFeeAmount, address refundAddress);

contracts/interfaces/IAxelarGateway.sol
32-50:
    event TokenSent(address indexed sender, string destinationChain, string destinationAddress, string symbol, uint256 amount);

    event ContractCall(
        address indexed sender,
        string destinationChain,
        string destinationContractAddress,
        bytes32 indexed payloadHash,
        bytes payload
    );

    event ContractCallWithToken(
        address indexed sender,
        string destinationChain,
        string destinationContractAddress,
        bytes32 indexed payloadHash,
        bytes payload,
        string symbol,
        uint256 amount
    );

Unused/empty receive()/fallback() function

Sometimes Ether might be mistakenly sent to the contract. It's better to prevent this from happening.

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

contracts/deposit-service/AxelarDepositServiceProxy.sol
    receive() external payable override {}

contracts/deposit-service/DepositReceiver.sol
    receive() external payable {}

NATSPEC not complete

contracts/gas-service/AxelarGasService.sol

Suggestion: Follow NATSPEC.

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

contracts/AxelarGateway.sol
207:
        for (uint256 i = 0; i < symbols.length; i++) {

contracts/auth/AxelarAuthWeighted.sol
98:
        for (uint256 i = 0; i < signatures.length; ++i) {
116:
        for (uint256 i; i < accounts.length - 1; ++i) {

contracts/deposit-service/AxelarDepositService.sol
114:
        for (uint256 i; i < refundTokens.length; i++) {
168:
        for (uint256 i; i < refundTokens.length; i++) {
204:
        for (uint256 i; i < refundTokens.length; i++) {

contracts/gas-service/AxelarGasService.sol
123:
        for (uint256 i; i < tokens.length; i++) {

Suggestion: Cache the length before the loop.

    uint length = names.length;

FOR-LOOP unchecked{++i } COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

contracts/AdminMultisigBase.sol
51:
        for (uint256 i; i < adminCount; ++i) {
158:
        for (uint256 i; i < adminLength; ++i) {

contracts/AxelarGateway.sol
195:
        for (uint256 i; i < adminCount; ++i) {
207:
        for (uint256 i = 0; i < symbols.length; i++) {
292:
        for (uint256 i; i < commandsLength; ++i) {

contracts/auth/AxelarAuthWeighted.sol
17:
        for (uint256 i; i < recentOperators.length; ++i) {
69:
        for (uint256 i = 0; i < weightsLength; ++i) {
98:
        for (uint256 i = 0; i < signatures.length; ++i) {
101:
        for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
116:
        for (uint256 i; i < accounts.length - 1; ++i) {

contracts/deposit-service/AxelarDepositService.sol
114:
        for (uint256 i; i < refundTokens.length; i++) {
168:
        for (uint256 i; i < refundTokens.length; i++) {
204:
        for (uint256 i; i < refundTokens.length; i++) {

contracts/gas-service/AxelarGasService.sol
123:
        for (uint256 i; i < tokens.length; i++) {

Suggestion: For readability, uncheck the whole for loop is the same.

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

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

contracts/AxelarGateway.sol
207:
        for (uint256 i = 0; i < symbols.length; i++) {

contracts/auth/AxelarAuthWeighted.sol
69:
        for (uint256 i = 0; i < weightsLength; ++i) {
98:
        for (uint256 i = 0; i < signatures.length; ++i) {
94-95:
        uint256 operatorIndex = 0;
        uint256 weight = 0;

The demo of the loop gas comparison can be seen here.

X = X + Y IS CHEAPER THAN X += Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y to save gas

contracts/auth/AxelarAuthWeighted.sol

70:         totalWeight += newWeights[i];
105:        weight += weights[operatorIndex];

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as onlyOwner() is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

The extra opcodes avoided are

CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)

which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

modifier onlyAdmin contracts/AxelarGateway.sol

204:    function setTokenDailyMintLimits() external override onlyAdmin {}
217:    function upgrade() external override onlyAdmin {}

modifier onlySelf contracts/AxelarGateway.sol

331:    function deployToken() external onlySelf {}
367:    function mintToken() external onlySelf {}
373:    function burnToken() external onlySelf {}
397:    function approveContractCall() external onlySelf {}
411:    function approveContractCallWithMint() external onlySelf {}
437:    function transferOperatorship() external onlySelf {}

modifier onlyOwner

contracts/BurnableMintableCappedERC20.sol
34:     function burn(bytes32 salt) external onlyOwner {}
39:     function burnFrom() external onlyOwner {}

contracts/MintableCappedERC20.sol
23:     function mint() external onlyOwner {}

contracts/auth/AxelarAuthWeighted.sol
47:     function transferOperatorship() external onlyOwner {}
120:    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {}
136:    function refund() external onlyOwner {}

xc20/contracts/XC20Wrapper.sol
44:     function setXc20Codehash(bytes32 newCodehash) external onlyOwner {}
48:     function addWrapping() external payable onlyOwner {}
66:     function removeWrapping() external onlyOwner {}

gatewayToken and wrappedTokenAddress assignment can be moved before the for loop

contracts/deposit-service/AxelarDepositService.sol

106-135:
    function refundTokenDeposit(,,,, string calldata tokenSymbol,) external {
        for (uint256 i; i < refundTokens.length; i++) {
            address gatewayToken = IAxelarGateway(gateway).tokenAddresses(tokenSymbol);
        // ...
        }
    }

198-218:
    function refundNativeUnwrap(
        bytes32 salt,
        address refundAddress,
        address payable recipient,
        address[] calldata refundTokens
    ) external {
        for (uint256 i; i < refundTokens.length; i++) {
            address wrappedTokenAddress = wrappedToken();
            // ...
        }

        refundToken = address(0);
    }

gatewayToken and wrappedTokenAddress won't change for every iteration, it can be moved before the for loop.