Platform: Code4rena
Start Date: 29/07/2022
Pot Size: $50,000 USDC
Total HM: 6
Participants: 75
Period: 5 days
Judge: GalloDaSballo
Total Solo HM: 3
Id: 149
League: ETH
Rank: 10/75
Findings: 2
Award: $529.69
š Selected for report: 1
š Solo Findings: 0
š Selected for report: oyc_109
Also found by: 0x1f8b, 0x52, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, IllIllI, JC, Lambda, Noah3o6, NoamYakov, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Twpony, Waze, Yiko, __141345__, ajtra, apostle0x01, ashiq0x01, asutorufos, bardamu, benbaessler, berndartmueller, bharg4v, bulej93, c3phas, cccz, ch13fd357r0y3r, codexploder, cryptonue, cryptphi, defsec, djxploit, durianSausage, fatherOfBlocks, gogo, hansfriese, horsefacts, ignacio, kyteg, lucacez, mics, rbserver, robee, sashik_eth, simon135, sseefried, tofunmi, xiaoming90
224.1386 USDC - $224.14
Issue | Instances | |
---|---|---|
[Lā01] | Don't use payable.transfer() /payable.send() | 4 |
[Lā02] | Unused/empty receive() /fallback() function | 2 |
[Lā03] | Missing checks for address(0x0) when assigning values to address state variables | 2 |
[Lā04] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 5 |
Total: 13 instances over 4 issues
Issue | Instances | |
---|---|---|
[Nā01] | Return values of approve() not checked | 3 |
[Nā02] | public functions not called by the contract should be declared external instead | 1 |
[Nā03] | constant s should be defined rather than using magic numbers | 2 |
[Nā04] | Use a more recent version of solidity | 2 |
[Nā05] | Expressions for constant values such as a call to keccak256() , should use immutable rather than constant | 13 |
[Nā06] | File is missing NatSpec | 10 |
[Nā07] | Event is missing indexed fields | 7 |
Total: 38 instances over 7 issues
payable.transfer()
/payable.send()
The use of payable.transfer()
is heavily frowned upon because it can lead to the locking of funds. The transfer()
call requires that the recipient is either an EOA account, or is a contract that has a payable
callback. For the contract case, the transfer()
call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:
payable
callbackpayable
callback spends more than 2300 gas (which is only enough to emit something)Address.sendValue()
insteadThere are 4 instances of this issue:
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);
receive()
/fallback()
functionIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))
)
There are 2 instances of this issue:
File: contracts/deposit-service/AxelarDepositServiceProxy.sol 13: receive() external payable override {}
File: contracts/deposit-service/DepositReceiver.sol 29: receive() external payable {}
address(0x0)
when assigning values to address
state variablesThere are 2 instances of this issue:
File: contracts/AxelarGateway.sol 52: AUTH_MODULE = authModule; 53: TOKEN_DEPLOYER_IMPLEMENTATION = tokenDeployerImplementation;
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.
If all arguments are strings and or bytes, bytes.concat()
should be used instead
There are 5 instances of this issue:
File: contracts/AxelarGateway.sol 342: bytes32 salt = keccak256(abi.encodePacked(symbol)); 540: return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_LIMIT, symbol)); 544: return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day)); 548: return keccak256(abi.encodePacked(PREFIX_TOKEN_TYPE, symbol)); 552: return keccak256(abi.encodePacked(PREFIX_TOKEN_ADDRESS, symbol));
approve()
not checkedNot all IERC20
implementations revert()
when there's a failure in approve()
. The function signature has a boolean
return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
There are 3 instances of this issue:
File: contracts/deposit-service/AxelarDepositService.sol 30: IERC20(wrappedTokenAddress).approve(gateway, amount);
File: contracts/deposit-service/ReceiverImplementation.sol 38: IERC20(tokenAddress).approve(gateway, amount); 64: IERC20(wrappedTokenAddress).approve(gateway, amount);
public
functions not called by the contract should be declared external
insteadContracts are allowed to override their parents' functions and change the visibility from external
to public
.
There is 1 instance of this issue:
File: contracts/deposit-service/AxelarDepositService.sol 241: function contractId() public pure returns (bytes32) {
constant
s should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 2 instances of this issue:
File: contracts/deposit-service/AxelarDepositService.sol /// @audit 0xff 229: bytes1(0xff),
File: contracts/deposit-service/DepositReceiver.sol /// @audit 0x40 18: let ptr := mload(0x40)
Use a solidity version of at least 0.8.12 to get string.concat()
to be used instead of abi.encodePacked(<str>,<str>)
There are 2 instances of this issue:
File: contracts/AxelarGateway.sol 3: pragma solidity 0.8.9;
File: contracts/deposit-service/AxelarDepositService.sol 3: pragma solidity 0.8.9;
keccak256()
, should use immutable
rather than constant
While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant
variables and immutable
variables, and they should each be used in their appropriate contexts. constants
should be used for literal values written into the code, and immutable
variables should be used for expressions, or values calculated in, or passed into the constructor.
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');
There are 10 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol
File: contracts/deposit-service/AxelarDepositServiceProxy.sol
File: contracts/deposit-service/DepositReceiver.sol
File: contracts/gas-service/AxelarGasServiceProxy.sol
File: contracts/gas-service/AxelarGasService.sol
File: contracts/interfaces/IAxelarAuth.sol
File: contracts/interfaces/IAxelarAuthWeighted.sol
File: contracts/interfaces/IAxelarDepositService.sol
File: contracts/interfaces/IAxelarExecutable.sol
File: contracts/interfaces/IAxelarGasService.sol
indexed
fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event
should use three indexed
fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 7 instances of this issue:
File: contracts/interfaces/IAxelarAuthWeighted.sol 14: event OperatorshipTransferred(address[] newOperators, uint256[] newWeights, uint256 newThreshold);
File: contracts/interfaces/IAxelarGasService.sol 13 event GasPaidForContractCall( 14 address indexed sourceAddress, 15 string destinationChain, 16 string destinationAddress, 17 bytes32 indexed payloadHash, 18 address gasToken, 19 uint256 gasFeeAmount, 20 address refundAddress 21: ); 23 event GasPaidForContractCallWithToken( 24 address indexed sourceAddress, 25 string destinationChain, 26 string destinationAddress, 27 bytes32 indexed payloadHash, 28 string symbol, 29 uint256 amount, 30 address gasToken, 31 uint256 gasFeeAmount, 32 address refundAddress 33: ); 35 event NativeGasPaidForContractCall( 36 address indexed sourceAddress, 37 string destinationChain, 38 string destinationAddress, 39 bytes32 indexed payloadHash, 40 uint256 gasFeeAmount, 41 address refundAddress 42: ); 44 event NativeGasPaidForContractCallWithToken( 45 address indexed sourceAddress, 46 string destinationChain, 47 string destinationAddress, 48 bytes32 indexed payloadHash, 49 string symbol, 50 uint256 amount, 51 uint256 gasFeeAmount, 52 address refundAddress 53: ); 55: event GasAdded(bytes32 indexed txHash, uint256 indexed logIndex, address gasToken, uint256 gasFeeAmount, address refundAddress); 57: event NativeGasAdded(bytes32 indexed txHash, uint256 indexed logIndex, uint256 gasFeeAmount, address refundAddress);
#0 - re1ro
2022-08-05T06:17:28Z
Dup #4
Dup #3 (5)
Dup #8 (5)
Dup #3
Ack
Ack
Dup #3
Dup #12
Ack
Dup #3
#1 - GalloDaSballo
2022-08-31T23:46:49Z
L
Valid for DepositServiceProxy L
L
Disputed for the cases prevented, the only demo I've ever found looked like the following
privilegedAccounts = [0x1, 0x2] plebAccounts = [0x3]
newPriveleged = [0x1] newPlebs = [0x2, 0x3]
abi.encodePacked(privilegedAccounts, plebAccounts) == abi.encodePacked(newPriveleged, newPlebs)
Raising to L because of the revert with certain tokens
R
##Ā [Nā03] constants should be defined rather than using magic numbers I'm going to dispute the 2 specific instances, while the first one can be argued, the second is the free memory pointer
R
Not true for a loooooooooooooong time -> https://twitter.com/GalloDaSballo/status/1543729080926871557
4L 2R
š Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xsam, 8olidity, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, JC, Lambda, MiloTruck, Noah3o6, NoamYakov, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Tomio, Waze, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, benbaessler, bharg4v, bulej93, c3phas, defsec, djxploit, durianSausage, erictee, fatherOfBlocks, gerdusx, gogo, kyteg, lucacez, medikko, mics, owenthurm, oyc_109, rbserver, robee, sashik_eth, simon135, tofunmi
305.5469 USDC - $305.55
Issue | Instances | |
---|---|---|
[Gā01] | Using calldata instead of memory for read-only arguments in external functions saves gas | 7 |
[Gā02] | Avoid contract existence checks by using solidity version 0.8.10 or later | 25 |
[Gā03] | internal functions only called once can be inlined to save gas | 7 |
[Gā04] | <array>.length should not be looked up in every loop of a for -loop | 7 |
[Gā05] | ++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 | 12 |
[Gā06] | keccak256() should only need to be called on a specific string literal once | 4 |
[Gā07] | Optimize names to save gas | 10 |
[Gā08] | ++i costs less gas than i++ , especially when it's used in for -loops (--i /i-- too) | 5 |
[Gā09] | Empty blocks should be removed or emit something | 2 |
[Gā10] | Functions guaranteed to revert when called by normal users can be marked payable | 11 |
Total: 90 instances over 10 issues
calldata
instead of memory
for read-only arguments in external
functions saves gasWhen a function with a memory
array is called externally, the abi.decode()
step has to use a for-loop to copy each index of the calldata
to the memory
index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length
). Using calldata
directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory
arguments, it's still valid for implementation contracs to use calldata
arguments instead.
If the array is passed to an internal
function which passes the array to another internal function where the array is modified and therefore memory
is used in the external
call, it's still more gass-efficient to use calldata
when the external
function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public
but can be marked as external
since it's not called by the contract, and cases where a constructor is involved
There are 7 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol /// @audit recentOperators 16: constructor(bytes[] memory recentOperators) {
File: contracts/AxelarGateway.sol 172: function tokenFrozen(string memory) external pure override returns (bool) { /// @audit executeData 447 function _unpackLegacyCommands(bytes memory executeData) 448 external 449 pure 450 returns ( 451 uint256 chainId, 452 bytes32[] memory commandIds, 453 string[] memory commands, 454: bytes[] memory params
File: contracts/deposit-service/AxelarDepositService.sol /// @audit wrappedSymbol 18: constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {
File: contracts/deposit-service/DepositReceiver.sol /// @audit delegateData 8: constructor(bytes memory delegateData) {
File: contracts/deposit-service/ReceiverImplementation.sol /// @audit wrappedSymbol 12: constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {}
File: contracts/gas-service/AxelarGasService.sol /// @audit symbol 35 function payGasForContractCallWithToken( 36 address sender, 37 string calldata destinationChain, 38 string calldata destinationAddress, 39 bytes calldata payload, 40 string memory symbol, 41 uint256 amount, 42 address gasToken, 43 uint256 gasFeeAmount, 44: address refundAddress
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE
(100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 25 instances of this issue:
File: contracts/AxelarGateway.sol /// @audit validateProof() 268: bool currentOperators = IAxelarAuth(AUTH_MODULE).validateProof(messageHash, proof); /// @audit _unpackLegacyCommands() 275: try AxelarGateway(this)._unpackLegacyCommands(data) returns ( /// @audit call() 320: (bool success, ) = address(this).call(abi.encodeWithSelector(commandSelector, params[i], commandId)); /// @audit balanceOf() 385: abi.encodeWithSelector(IERC20.transfer.selector, address(this), IERC20(tokenAddress).balanceOf(address(depositHandler))) /// @audit burn() 393: IBurnableMintableCappedERC20(tokenAddress).burn(salt); /// @audit mint() 481: IBurnableMintableCappedERC20(tokenAddress).mint(account, amount); /// @audit depositAddress() 525: IBurnableMintableCappedERC20(tokenAddress).depositAddress(bytes32(0)), /// @audit burn() 532: IBurnableMintableCappedERC20(tokenAddress).burn(bytes32(0));
File: contracts/deposit-service/AxelarDepositService.sol /// @audit approve() 30: IERC20(wrappedTokenAddress).approve(gateway, amount); /// @audit tokenAddresses() 115: address gatewayToken = IAxelarGateway(gateway).tokenAddresses(tokenSymbol);
File: contracts/deposit-service/DepositReceiver.sol /// @audit delegatecall() /// @audit receiverImplementation() 12: (bool success, ) = IAxelarDepositService(msg.sender).receiverImplementation().delegatecall(delegateData);
File: contracts/deposit-service/ReceiverImplementation.sol /// @audit tokenAddresses() 25: address tokenAddress = IAxelarGateway(gateway).tokenAddresses(symbol); /// @audit refundToken() 27: address refund = DepositBase(msg.sender).refundToken(); /// @audit balanceOf() 29: _safeTransfer(refund, refundAddress, IERC20(refund).balanceOf(address(this))); /// @audit balanceOf() 33: uint256 amount = IERC20(tokenAddress).balanceOf(address(this)); /// @audit approve() 38: IERC20(tokenAddress).approve(gateway, amount); /// @audit refundToken() 49: address refund = DepositBase(msg.sender).refundToken(); /// @audit balanceOf() 53: _safeTransfer(refund, refundAddress, IERC20(refund).balanceOf(address(this))); /// @audit approve() 64: IERC20(wrappedTokenAddress).approve(gateway, amount); /// @audit refundToken() 74: address refund = DepositBase(msg.sender).refundToken(); /// @audit balanceOf() 76: _safeTransfer(refund, refundAddress, IERC20(refund).balanceOf(address(this))); /// @audit balanceOf() 80: uint256 amount = IERC20(wrappedTokenAddress).balanceOf(address(this)); /// @audit withdraw() 85: IWETH9(wrappedTokenAddress).withdraw(amount);
File: contracts/gas-service/AxelarGasService.sol /// @audit balanceOf() 130: uint256 amount = IERC20(token).balanceOf(address(this));
internal
functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP
instructions and additional stack operations needed for function calls.
There are 7 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol 86 function _validateSignatures( 87 bytes32 messageHash, 88 address[] memory operators, 89 uint256[] memory weights, 90 uint256 threshold, 91: bytes[] memory signatures 115: function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {
File: contracts/AxelarGateway.sol 611: function _setTokenDailyMintAmount(string memory symbol, uint256 amount) internal { 622: function _setTokenAddress(string memory symbol, address tokenAddress) internal { 630 function _setContractCallApproved( 631 bytes32 commandId, 632 string memory sourceChain, 633 string memory sourceAddress, 634 address contractAddress, 635: bytes32 payloadHash 640 function _setContractCallApprovedWithMint( 641 bytes32 commandId, 642 string memory sourceChain, 643 string memory sourceAddress, 644 address contractAddress, 645 bytes32 payloadHash, 646 string memory symbol, 647: uint256 amount 655: function _setImplementation(address newImplementation) internal {
<array>.length
should not be looked up in every loop of a for
-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD
(3 gas)CALLDATALOAD
(3 gas)Caching the length changes each of these to a DUP<N>
(3 gas), and gets rid of the extra DUP<N>
needed to store the stack offset
There are 7 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol 17: for (uint256 i; i < recentOperators.length; ++i) { 98: for (uint256 i = 0; i < signatures.length; ++i) {
File: contracts/AxelarGateway.sol 207: for (uint256 i = 0; i < symbols.length; i++) {
File: contracts/deposit-service/AxelarDepositService.sol 114: for (uint256 i; i < refundTokens.length; i++) { 168: for (uint256 i; i < refundTokens.length; i++) { 204: for (uint256 i; i < refundTokens.length; i++) {
File: contracts/gas-service/AxelarGasService.sol 123: for (uint256 i; i < tokens.length; i++) {
++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
-loopsThe 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
There are 12 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol 17: for (uint256 i; i < recentOperators.length; ++i) { 69: for (uint256 i = 0; i < weightsLength; ++i) { 98: for (uint256 i = 0; i < signatures.length; ++i) { 101: for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {} 116: for (uint256 i; i < accounts.length - 1; ++i) {
File: contracts/AxelarGateway.sol 195: for (uint256 i; i < adminCount; ++i) { 207: for (uint256 i = 0; i < symbols.length; i++) { 292: for (uint256 i; i < commandsLength; ++i) {
File: contracts/deposit-service/AxelarDepositService.sol 114: for (uint256 i; i < refundTokens.length; i++) { 168: for (uint256 i; i < refundTokens.length; i++) { 204: for (uint256 i; i < refundTokens.length; i++) {
File: contracts/gas-service/AxelarGasService.sol 123: for (uint256 i; i < tokens.length; i++) {
keccak256()
should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4
should also only be done once
There are 4 instances of this issue:
File: contracts/deposit-service/AxelarDepositServiceProxy.sol 9: return keccak256('axelar-deposit-service');
File: contracts/deposit-service/AxelarDepositService.sol 242: return keccak256('axelar-deposit-service');
File: contracts/gas-service/AxelarGasServiceProxy.sol 10: return keccak256('axelar-gas-service');
File: contracts/gas-service/AxelarGasService.sol 181: return keccak256('axelar-gas-service');
public
/external
function names and public
member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 10 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol /// @audit validateProof(), transferOperatorship() 9: contract AxelarAuthWeighted is Ownable, IAxelarAuthWeighted {
File: contracts/AxelarGateway.sol /// @audit sendToken(), callContract(), callContractWithToken(), deployToken(), mintToken(), burnToken(), approveContractCall(), approveContractCallWithMint(), transferOperatorship(), _unpackLegacyCommands() 15: contract AxelarGateway is IAxelarGateway, AdminMultisigBase {
File: contracts/deposit-service/AxelarDepositService.sol /// @audit sendNative(), addressForTokenDeposit(), addressForNativeDeposit(), addressForNativeUnwrap(), sendTokenDeposit(), refundTokenDeposit(), sendNativeDeposit(), refundNativeDeposit(), nativeUnwrap(), refundNativeUnwrap(), contractId() 15: contract AxelarDepositService is Upgradable, DepositBase, IAxelarDepositService {
File: contracts/deposit-service/ReceiverImplementation.sol /// @audit receiveAndSendToken(), receiveAndSendNative(), receiveAndUnwrapNative() 11: contract ReceiverImplementation is DepositBase {
File: contracts/gas-service/AxelarGasService.sol /// @audit collectFees(), refund(), contractId() 10: contract AxelarGasService is Upgradable, IAxelarGasService {
File: contracts/interfaces/IAxelarAuth.sol /// @audit validateProof(), transferOperatorship() 7: interface IAxelarAuth is IOwnable {
File: contracts/interfaces/IAxelarAuthWeighted.sol /// @audit currentEpoch(), hashForEpoch(), epochForHash() 7: interface IAxelarAuthWeighted is IAxelarAuth {
File: contracts/interfaces/IAxelarDepositService.sol /// @audit sendNative(), addressForTokenDeposit(), addressForNativeDeposit(), addressForNativeUnwrap(), sendTokenDeposit(), refundTokenDeposit(), sendNativeDeposit(), refundNativeDeposit(), nativeUnwrap(), refundNativeUnwrap(), receiverImplementation() 9: interface IAxelarDepositService is IUpgradable, IDepositBase {
File: contracts/interfaces/IAxelarExecutable.sol /// @audit execute(), executeWithToken() 7: abstract contract IAxelarExecutable {
File: contracts/interfaces/IAxelarGasService.sol /// @audit payGasForContractCall(), payGasForContractCallWithToken(), payNativeGasForContractCall(), payNativeGasForContractCallWithToken(), addGas(), addNativeGas(), collectFees(), refund() 8: interface IAxelarGasService is IUpgradable {
++i
costs less gas than i++
, especially when it's used in for
-loops (--i
/i--
too)Saves 5 gas per loop
There are 5 instances of this issue:
File: contracts/AxelarGateway.sol 207: for (uint256 i = 0; i < symbols.length; i++) {
File: contracts/deposit-service/AxelarDepositService.sol 114: for (uint256 i; i < refundTokens.length; i++) { 168: for (uint256 i; i < refundTokens.length; i++) { 204: for (uint256 i; i < refundTokens.length; i++) {
File: contracts/gas-service/AxelarGasService.sol 123: for (uint256 i; i < tokens.length; i++) {
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. If the contract is meant to be extended, the contract should be abstract
and the function signatures be added without any default implementation. If the block is an empty if
-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...}
=> if(!x){if(y){...}else{...}}
). Empty receive()
/fallback() payable
functions that are not used, can be removed to save deployment gas.
There are 2 instances of this issue:
File: contracts/interfaces/IAxelarExecutable.sol 46: ) internal virtual {} 54: ) internal virtual {}
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
There are 11 instances of this issue:
File: contracts/auth/AxelarAuthWeighted.sol 47: function transferOperatorship(bytes calldata params) external onlyOwner {
File: contracts/AxelarGateway.sol 204: function setTokenDailyMintLimits(string[] calldata symbols, uint256[] calldata limits) external override onlyAdmin { 217 function upgrade( 218 address newImplementation, 219 bytes32 newImplementationCodeHash, 220 bytes calldata setupParams 221: ) external override onlyAdmin { 331: function deployToken(bytes calldata params, bytes32) external onlySelf { 367: function mintToken(bytes calldata params, bytes32) external onlySelf { 373: function burnToken(bytes calldata params, bytes32) external onlySelf { 397: function approveContractCall(bytes calldata params, bytes32 commandId) external onlySelf { 411: function approveContractCallWithMint(bytes calldata params, bytes32 commandId) external onlySelf { 437: function transferOperatorship(bytes calldata newOperatorsData, bytes32) external onlySelf {
File: contracts/gas-service/AxelarGasService.sol 120: function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner { 136 function refund( 137 address payable receiver, 138 address token, 139 uint256 amount 140: ) external onlyOwner {
#0 - re1ro
2022-08-05T06:18:25Z
Dup #2 #3
#1 - GalloDaSballo
2022-08-25T01:28:08Z
60 for the array of bytes
100 gas per instance 2500
20 per instance 140
Giving 300 consistently with rest of submissions
30 gas per instance 120
Rest is too opinionated for me :P
Great report as usual, would love to see a couple customized suggestion (packing or similar) and benchmarks, but still really good
3120 gas saved