Axelar Network v2 contest - durianSausage's results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/ReceiverImplementation.sol#L23 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/ReceiverImplementation.sol#L51 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/ReceiverImplementation.sol#L71 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/ReceiverImplementation.sol#L86 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L128 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L144

Vulnerability details

problem

We should use address(xxx).call{value:xxxx} instead of payable(msg.sender).transfer . Because if msg.sender is a contract, it can use its receive function to reentrance attacking or using some logic vulner abilities to lock the entire contract.

prof:

'deposit-service\ReceiverImplementation.sol', 22, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'deposit-service\ReceiverImplementation.sol', 50, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'deposit-service\ReceiverImplementation.sol', 70, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'deposit-service\ReceiverImplementation.sol', 85, ' recipient.transfer(amount); 'gas-service\AxelarGasService.sol', 127, ' if (amount > 0) receiver.transfer(amount); 'gas-service\AxelarGasService.sol', 143, ' receiver.transfer(amount);

mitigation

'deposit-service\ReceiverImplementation.sol', 22, ' if (address(this).balance > 0) refundAddress.call{value:address(this).balance}(); 'deposit-service\ReceiverImplementation.sol', 50, ' if (address(this).balance > 0) refundAddress.call{value:address(this).balance}(); 'deposit-service\ReceiverImplementation.sol', 70, ' if (address(this).balance > 0) refundAddress.call{value:address(this).balance}(); 'deposit-service\ReceiverImplementation.sol', 85, ' recipient.call{value:amount}(); 'gas-service\AxelarGasService.sol', 127, ' if (amount > 0) receiver.call{value:amount}(); 'gas-service\AxelarGasService.sol', 143, ' receiver.call{value:amount}();

gas optimization

G01: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

'AxelarGateway.sol', 206, ' for (uint256 i = 0; i < symbols.length; i++) { 'ERC20Permit.sol', 52, ' keccak256(abi.encode(PERMIT_SIGNATURE_HASH, issuer, spender, value, nonces[issuer]++, deadline)) 'deposit-service\AxelarDepositService.sol', 113, ' for (uint256 i; i < refundTokens.length; i++) { 'deposit-service\AxelarDepositService.sol', 167, ' for (uint256 i; i < refundTokens.length; i++) { 'deposit-service\AxelarDepositService.sol', 203, ' for (uint256 i; i < refundTokens.length; i++) { 'gas-service\AxelarGasService.sol', 122, ' for (uint256 i; i < tokens.length; i++) {

G02: USING BOOLS FOR STORAGE INCURS OVERHEAD

problem

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

prof

'EternalStorage.sol', 13, ' mapping(bytes32 => bool) private _boolStorage;

G03: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

'AxelarGateway.sol', 612, ' if (limit > 0 && amount > limit) revert ExceedDailyMintLimit(symbol); 'AxelarGateway.sol', 254, ' if (newOperatorsData.length > 0) { 'auth\AxelarAuthWeighted.sol', 75, ' if (epochForHash[newOperatorsHash] > 0) revert SameOperators(); 'deposit-service\AxelarDepositService.sol', 164, ' if (addressForNativeDeposit(salt, refundAddress, destinationChain, destinationAddress).balance > 0 && msg.sender != refundAddress) 'deposit-service\ReceiverImplementation.sol', 22, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'deposit-service\ReceiverImplementation.sol', 50, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'deposit-service\ReceiverImplementation.sol', 70, ' if (address(this).balance > 0) refundAddress.transfer(address(this).balance); 'gas-service\AxelarGasService.sol', 127, ' if (amount > 0) receiver.transfer(amount); 'gas-service\AxelarGasService.sol', 130, ' if (amount > 0) _safeTransfer(token, receiver, amount); 'util\Upgradable.sol', 49, ' if (params.length > 0) {

G04: resign the default value to the variables.

problem

resign the default value to the variables will cost more gas.

prof

'AxelarGateway.sol', 206, ' for (uint256 i = 0; i < symbols.length; i++) { 'auth\AxelarAuthWeighted.sol', 67, ' uint256 totalWeight = 0; 'auth\AxelarAuthWeighted.sol', 68, ' for (uint256 i = 0; i < weightsLength; ++i) { 'auth\AxelarAuthWeighted.sol', 93, ' uint256 operatorIndex = 0; 'auth\AxelarAuthWeighted.sol', 94, ' uint256 weight = 0; 'auth\AxelarAuthWeighted.sol', 97, ' for (uint256 i = 0; i < signatures.length; ++i) {

G05: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

'ERC20.sol', 171, ' balanceOf[sender] -= amount; 'ERC20.sol', 172, ' balanceOf[recipient] += amount; 'ERC20.sol', 190, ' totalSupply += amount; 'ERC20.sol', 191, ' balanceOf[account] += amount; 'ERC20.sol', 211, ' balanceOf[account] -= amount; 'ERC20.sol', 212, ' totalSupply -= amount;