code-423n4 / 2022-07-axelar-findings

0 stars 0 forks source link

QA Report #209

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

It is expected that the value should be converted into a constant value at compile time, but actually the expression is re-calculated each time the constant is referenced

There are 13 instances of this issue:

File: contracts/AxelarGateway.sol

30:   bytes32 internal constant PREFIX_COMMAND_EXECUTED = keccak256('command-executed');

31:   bytes32 internal constant PREFIX_TOKEN_ADDRESS = keccak256('token-address');

32:   bytes32 internal constant PREFIX_TOKEN_TYPE = keccak256('token-type');

33:   bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED = keccak256('contract-call-approved');

34:   bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED_WITH_MINT = keccak256('contract-call-approved-with-mint');

35:   bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_LIMIT = keccak256('token-daily-mint-limit');

36:   bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_AMOUNT = keccak256('token-daily-mint-amount');

38:   bytes32 internal constant SELECTOR_BURN_TOKEN = keccak256('burnToken');

39:   bytes32 internal constant SELECTOR_DEPLOY_TOKEN = keccak256('deployToken');

40:   bytes32 internal constant SELECTOR_MINT_TOKEN = keccak256('mintToken');

41:   bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL = keccak256('approveContractCall');

42:   bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL_WITH_MINT = keccak256('approveContractCallWithMint');

43:   bytes32 internal constant SELECTOR_TRANSFER_OPERATORSHIP = keccak256('transferOperatorship');

https://github.com/code-423n4/2022-07-axelar/tree/main/contracts/AxelarGateway.sol#L30-L43

Use .call() instead of .transfer()

Since .tranfer() limits the allowed gas to 2300 units in the receiver's receive() or fallback() function it might result in DoS if the receiver is a contract address and has some logic in the receive() or fallback() function. Use .call{value: x}( and nonreentrant modifier or Checks-Effects-Interactions pattern.

There are 6 instances of this issue:

File: contracts/gas-service/AxelarGasService.sol

128: if (amount > 0) receiver.transfer(amount);

144: receiver.transfer(amount);

https://github.com/code-423n4/2022-07-axelar/tree/main/contracts/gas-service/AxelarGasService.sol#L128

File: contracts/deposit-service/ReceiverImplementation.sol

23:  if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

51:  if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

71:  if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

86:  recipient.transfer(amount);

https://github.com/code-423n4/2022-07-axelar/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L23

GalloDaSballo commented 2 years ago

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Nooooooooooooooo https://twitter.com/GalloDaSballo/status/1543729080926871557

Use .call() instead of .transfer()

Valid Low

although with every new exploit I'm starting to think transfer is the better option