Axelar Network v2 contest - oyc_109's results

[L-01] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
IDepositBase.sol::3 => pragma solidity ^0.8.9;

[L-02] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

AxelarGateway.sol::157 => return getUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days));
AxelarGateway.sol::615 => _setUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days), amount);

[L-03] Unused receive() function

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

AxelarDepositServiceProxy.sol::13 => receive() external payable override {}
DepositReceiver.sol::29 => receive() external payable {}

[L-04] decimals() not part of ERC20 standard

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

XC20Wrapper.sol::62 => if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). Unless there is a compelling reason, abi.encode should be preferred. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

AxelarDepositService.sol::233 => keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
AxelarGateway.sol::298 => bytes32 commandHash = keccak256(abi.encodePacked(commands[i]));
AxelarGateway.sol::342 => bytes32 salt = keccak256(abi.encodePacked(symbol));
AxelarGateway.sol::540 => return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_LIMIT, symbol));
AxelarGateway.sol::544 => return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day));
AxelarGateway.sol::548 => return keccak256(abi.encodePacked(PREFIX_TOKEN_TYPE, symbol));
AxelarGateway.sol::552 => return keccak256(abi.encodePacked(PREFIX_TOKEN_ADDRESS, symbol));
AxelarGateway.sol::556 => return keccak256(abi.encodePacked(PREFIX_COMMAND_EXECUTED, commandId));

[L-06] approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

approve is subject to a known front-running attack. Consider using safeApprove() or safeIncreaseAllowance() or safeDecreaseAllowance() instead

AxelarDepositService.sol::30 => IERC20(wrappedTokenAddress).approve(gateway, amount);
ReceiverImplementation.sol::38 => IERC20(tokenAddress).approve(gateway, amount);
ReceiverImplementation.sol::64 => IERC20(wrappedTokenAddress).approve(gateway, amount);

[L-07] Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

AxelarGasService.sol::128 => if (amount > 0) receiver.transfer(amount);
AxelarGasService.sol::144 => receiver.transfer(amount);
ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::86 => recipient.transfer(amount);

[L-08] Missing checks for zero address

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/AxelarGateway.sol#L229
https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L19

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

AxelarAuthWeighted.sol::3 => pragma solidity 0.8.9;
AxelarDepositServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarDepositService.sol::3 => pragma solidity 0.8.9;
AxelarGasServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarGasService.sol::3 => pragma solidity 0.8.9;
AxelarGateway.sol::3 => pragma solidity 0.8.9;
DepositBase.sol::3 => pragma solidity 0.8.9;
DepositReceiver.sol::3 => pragma solidity 0.8.9;
IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
IDepositBase.sol::3 => pragma solidity ^0.8.9;
ReceiverImplementation.sol::3 => pragma solidity 0.8.9;
XC20Wrapper.sol::3 => pragma solidity 0.8.9;

[N-02] require()/revert() statements should have descriptive reason strings

for troubleshotting purposes

XC20Wrapper.sol::55 => if (axelarToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::56 => if (xc20Token.codehash != xc20Codehash) revert('NotXc20Token()');
XC20Wrapper.sol::57 => if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');
XC20Wrapper.sol::58 => if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');
XC20Wrapper.sol::68 => if (axelarToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::70 => if (xc20Token == address(0)) revert('NotWrappingToken()');
XC20Wrapper.sol::78 => if (wrappedToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::84 => if (axelarToken == address(0)) revert('NotXc20Token()');
XC20Wrapper.sol::85 => if (IERC20(wrappedToken).balanceOf(msg.sender) < amount) revert('InsufficientBalance()');
XC20Wrapper.sol::98 => if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
XC20Wrapper.sol::111 => if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');

[N-03] Event is missing indexed fields

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

IAxelarAuthWeighted.sol::14 => event OperatorshipTransferred(address[] newOperators, uint256[] newWeights, uint256 newThreshold);

AxelarGateway.sol::544 => return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day));

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

AxelarAuthWeighted.sol::68 => uint256 totalWeight = 0;
AxelarAuthWeighted.sol::69 => for (uint256 i = 0; i < weightsLength; ++i) {
AxelarAuthWeighted.sol::94 => uint256 operatorIndex = 0;
AxelarAuthWeighted.sol::95 => uint256 weight = 0;
AxelarAuthWeighted.sol::98 => for (uint256 i = 0; i < signatures.length; ++i) {
AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {

[G-02] Cache Array Length Outside of Loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

AxelarAuthWeighted.sol::17 => for (uint256 i; i < recentOperators.length; ++i) {
AxelarAuthWeighted.sol::98 => for (uint256 i = 0; i < signatures.length; ++i) {
AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {
AxelarGasService.sol::123 => for (uint256 i; i < tokens.length; i++) {
AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {

[G-03] Use calldata instead of memory

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

AxelarAuthWeighted.sol::115 => function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {
AxelarDepositService.sol::220 => function _depositAddress(bytes32 create2Salt, bytes memory delegateData) internal view returns (address) {
AxelarGateway.sol::152 => function tokenDailyMintLimit(string memory symbol) public view override returns (uint256) {
AxelarGateway.sol::156 => function tokenDailyMintAmount(string memory symbol) public view override returns (uint256) {
AxelarGateway.sol::168 => function tokenAddresses(string memory symbol) public view override returns (address) {
AxelarGateway.sol::172 => function tokenFrozen(string memory) external pure override returns (bool) {
AxelarGateway.sol::460 => function _callERC20Token(address tokenAddress, bytes memory callData) internal returns (bool) {
AxelarGateway.sol::539 => function _getTokenDailyMintLimitKey(string memory symbol) internal pure returns (bytes32) {
AxelarGateway.sol::543 => function _getTokenDailyMintAmountKey(string memory symbol, uint256 day) internal pure returns (bytes32) {
AxelarGateway.sol::547 => function _getTokenTypeKey(string memory symbol) internal pure returns (bytes32) {
AxelarGateway.sol::551 => function _getTokenAddressKey(string memory symbol) internal pure returns (bytes32) {
AxelarGateway.sol::597 => function _getTokenType(string memory symbol) internal view returns (TokenType) {

[G-04] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

AxelarDepositService.sol::16 => address public immutable receiverImplementation;
DepositBase.sol::13 => address public immutable gateway;
XC20Wrapper.sol::24 => address public immutable gatewayAddress;

[G-05] 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

AxelarAuthWeighted.sol::47 => function transferOperatorship(bytes calldata params) external onlyOwner {
AxelarGateway.sol::204 => function setTokenDailyMintLimits(string[] calldata symbols, uint256[] calldata limits) external override onlyAdmin {
AxelarGateway.sol::331 => function deployToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol::367 => function mintToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol::373 => function burnToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol::397 => function approveContractCall(bytes calldata params, bytes32 commandId) external onlySelf {
AxelarGateway.sol::411 => function approveContractCallWithMint(bytes calldata params, bytes32 commandId) external onlySelf {
AxelarGateway.sol::437 => function transferOperatorship(bytes calldata newOperatorsData, bytes32) external onlySelf {
XC20Wrapper.sol::44 => function setXc20Codehash(bytes32 newCodehash) external onlyOwner {
XC20Wrapper.sol::66 => function removeWrapping(string calldata symbol) external onlyOwner {

[G-06] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

AxelarAuthWeighted.sol::101 => for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
AxelarDepositServiceProxy.sol::13 => receive() external payable override {}
DepositReceiver.sol::29 => receive() external payable {}

[G-07] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

AxelarAuthWeighted.sol::14 => uint8 internal constant OLD_KEY_RETENTION = 16;

[G-08] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

AxelarAuthWeighted.sol::17 => for (uint256 i; i < recentOperators.length; ++i) {
AxelarAuthWeighted.sol::69 => for (uint256 i = 0; i < weightsLength; ++i) {
AxelarAuthWeighted.sol::98 => for (uint256 i = 0; i < signatures.length; ++i) {
AxelarAuthWeighted.sol::101 => for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
AxelarAuthWeighted.sol::109 => ++operatorIndex;
AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {
AxelarGasService.sol::123 => for (uint256 i; i < tokens.length; i++) {
AxelarGateway.sol::195 => for (uint256 i; i < adminCount; ++i) {
AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {
AxelarGateway.sol::292 => for (uint256 i; i < commandsLength; ++i) {

[G-09] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

use <x> = <x> + <y> or <x> = <x> - <y> instead to save gas

AxelarAuthWeighted.sol::70 => totalWeight += newWeights[i];
AxelarAuthWeighted.sol::105 => weight += weights[operatorIndex];

[G-10] abi.encode() is less efficient than abi.encodePacked()

use abi.encodePacked() where possible to save gas

AxelarAuthWeighted.sol::32 => bytes32 operatorsHash = keccak256(abi.encode(operators, weights, threshold));
AxelarDepositService.sol::233 => keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
AxelarGateway.sol::566 => return keccak256(abi.encode(PREFIX_CONTRACT_CALL_APPROVED, commandId, sourceChain, sourceAddress, contractAddress, payloadHash));
AxelarGateway.sol::580 => abi.encode(

[G-11] Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

AxelarAuthWeighted.sol::3 => pragma solidity 0.8.9;
AxelarDepositServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarDepositService.sol::3 => pragma solidity 0.8.9;
AxelarGasServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarGasService.sol::3 => pragma solidity 0.8.9;
AxelarGateway.sol::3 => pragma solidity 0.8.9;
DepositBase.sol::3 => pragma solidity 0.8.9;
DepositReceiver.sol::3 => pragma solidity 0.8.9;
IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
IDepositBase.sol::3 => pragma solidity ^0.8.9;
ReceiverImplementation.sol::3 => pragma solidity 0.8.9;
XC20Wrapper.sol::3 => pragma solidity 0.8.9;

[G-12] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) Saves 6 gas PER LOOP

AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {
AxelarGasService.sol::123 => for (uint256 i; i < tokens.length; i++) {
AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {