Axelar Network v2 contest - Aymen0909's results

CONSTANTS SHOULD BE DEFINED RATHER THAN USING MAGIC NUMBERS

Impact - NON CRITICAL

File: contracts/deposit-service/DepositBase.sol

line 32 : if (symbolBytes.length == 0 || symbolBytes.length > 31) revert InvalidSymbol();
line 37 : symbolNumber |= 0xff & symbolBytes.length;
line 50 : uint256 length = 0xff & uint256(symbolData);

Unused/empty receive() function

Impact - LOW RISK

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

File: contracts/deposit-service/DepositReceiver.sol

line 29 : receive() external payable {}

File: contracts/deposit-service/AxelarDepositServiceProxy.so

line 13 : receive() external payable override {}

MISSING CHECKS FOR ADDRESS(0) BEFORE FUND TRANSFER

Impact - LOW RISK

Check if the receiver address is not zero address to avoid losing funds

File: contracts/deposit-service/ReceiverImplementation.sol

line 23, 51, 71 : if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

MISSING CHECKS FOR amount != 0 BEFORE FUND TRANSFER

Impact - LOW RISK

In the function mentioned below there is a need to check if the amount entered is different from 0 to avoid useless calls.

File: contracts/deposit-service/DepositReceiver.sol

line 75 : function wrap(address axelarToken, uint256 amount) external
line 82 : function unwrap(address wrappedToken, uint256 amount) external

I recommend using a check as follow :

if (amount == 0) revert InvalidAmount();

Use call() instead of transfer() when transferring ETH in RubiconRouter

Impact - LOW RISK

When transferring ETH, use call() instead of transfer().

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening.

Tihs issue present in the following lines :

File: xc20/contracts/XC20Wrapper.sol

line 63 : payable(msg.sender).transfer(address(this).balance);

File: contracts/deposit-service/ReceiverImplementation.sol

line 23, 51, 71 : if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

My recommendation is to replace transfer() calls with call(). Keep in mind to check whether the call was successful by validating the return value for example :

(bool success, ) = payable(msg.sender).call{value: address(this).balance}("");
if(!success) revert TransferFailed();

1- USE CALLDATA INSTEAD OF MEMORY FOR FUNCTION PARAMETERS TYPE :

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

File: xc20/contracts/XC20Wrapper.sol

line 48 : function addWrapping(string calldata symbol, address xc20Token, string memory newName, string memory newSymbol) external payable onlyOwner

File: contracts/auth/AxelarAuthWeighted.sol

line 86 : function _validateSignatures(bytes32 messageHash, address[] memory operators, uint256[] memory weights,uint256 threshold,bytes[] memory signatures) internal pure
line 115 : function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool)

2- 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 the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

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

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

File: contracts/AxelarGateway.sol

line 204 : function setTokenDailyMintLimits(string[] calldata symbols, uint256[] calldata limits) external override onlyAdmin
line 217 : function upgrade(address newImplementation, bytes32 newImplementationCodeHash, bytes calldata setupParams) external override onlyAdmin

File: xc20/contracts/XC20Wrapper.sol

line 66 : function removeWrapping(string calldata symbol) external onlyOwner

3- 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 or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

File: contracts/auth/AxelarAuthWeighted.sol

line 68 : uint256 totalWeight = 0;
line 69 : for (uint256 i = 0; i < weightsLength; ++i)
line 94 : uint256 operatorIndex = 0;
line 95 :  uint256 weight = 0;
line 98 : for (uint256 i = 0; i < signatures.length; ++i)

File: contracts/AxelarGateway.sol

line 207 : for (uint256 i = 0; i < symbols.length; i++)

4- USE OF ++i COST LESS GAS THAN i++ IN FOR LOOPS :

File: contracts/deposit-service/AxelarDepositService.sol

line 114, 168, 204 : for (uint256 i; i < refundTokens.length; i++)

File: contracts/AxelarGateway.sol

line 207 : for (uint256 i = 0; i < symbols.length; i++)

5- ++i/i++ SHOULD BE UNCHECKED{++i}/UNCHECKED{i++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR AND WHILE LOOPS

File: contracts/auth/AxelarAuthWeighted.sol

line 69 : for (uint256 i = 0; i < weightsLength; ++i)
line 98 : for (uint256 i = 0; i < signatures.length; ++i)

File: contracts/deposit-service/AxelarDepositService.sol

line 114, 168, 204 : for (uint256 i; i < refundTokens.length; i++)

File: contracts/AxelarGateway.sol

line 207 : for (uint256 i = 0; i < symbols.length; i++)