Axelar Network v2 contest - Respx's results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L76 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L36

Vulnerability details

Impact

It is possible to transfer operatorship to the same operators by simply doubling the values of the newWeights array and newThreshold value. This could be used by newly appointed operators to invalidate all previous operators and thus invalidate the intended functionality of AxelarAuthWeighted.OLD_KEY_RETENTION. It is possible operators might not do this maliciously: they might be testing or reconfiguring, unaware of the impact on the older commands. UPDATE: It has been explained on the Discord that "the operator address for a given epoch is derived from the validator key along with a nonce that is unique for each operator epoch and chain.". Given that, operators would be able to transfer operatorship to themselves indefinitely and would not need to even change weights.

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L76 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L36

Recommended Mitigation Steps

Maintain a state variable of the oldest allowable epoch, and create a command to update it.

Dangerous comment could break functionality

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/DepositBase.sol#L12 The comment on line 12 of DepositBase says that the relevant variables are immutable for gas optimisation. In fact, the immutable keyword is essential for functionality. The contract ReceiverImplementation inherits from DepositBase and is the target of delegatecall() from DepositReceiver. The functions called in these delegatecalls use the value of the variable gateway throughout. This variable would not be set in the storage of the calling DepositReceiver and it is only accessible because immutable variables are hardocded into the contract code by the constructor. That is why gateway is accessible in a delegatecall. If gateway were no longer immutable, it would take on the value in the corresponding storage slot of DepositReceiver.

Comment refers to ownership without ownership functionality

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/DepositBase.sol#L10 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L10 These comments refer to the contracts being "owned". Neither contract has any ownable functionality. Also, as they are used as targets for a delegatecall, the deployng account is not significant either.

AxelarAuthWeighted - Pointless Test

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L76 It has been explained on the Discord that "the operator address for a given epoch is derived from the validator key along with a nonce that is unique for each operator epoch and chain.". Given that, this test would not be able to detect if the operators were the same.

Commands in string form require unnecessary processing

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/AxelarGateway.sol#L271-L316 Because commands is an array of strings, the commands are being processed on line 298 into a bytes32 variable for easy comparison with constants. Functionally, however, these selectors are simply being used to switch between 6 options. It would be more gas efficient to submit the commands as uint variables or, at the very least, to submit them processed into bytes32 form. If necessary, it would be possible to have a function available for operators which converts the command strings to these uints. That function could be called in a gasless call when composing the transaction which calls execute().

Return a constant instead of keccak of a string literal

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L242 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositServiceProxy.sol#L9 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L181 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasServiceProxy.sol#L10 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L41 My gas tests showed that using a constant is more gas efficient than these keccak256() calls.

contract KeccakTest {
  bytes32 public constant myconstant = keccak256('axelar-deposit-service');
  function contractId() public pure returns (bytes32) {
      return keccak256('axelar-deposit-service');
  }
  function contractId2() public pure returns (bytes32) {
      return myconstant;
  }
}