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: 29/75
Findings: 2
Award: $88.39
🌟 Selected for report: 0
🚀 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
56.5067 USDC - $56.51
transfer() when sending Etherpublic functions not called by the contract should be declared external insteadtransfer() when sending Ethertransfer() uses a fixed 2300 gas. If the address where funds are transferred to uses more than this to handle the incoming Ether, the order transaction will revert. This limits the protocol to only interact with addresses where gas use upon receiving Ether is less than or equal to 2300 gas.
If for example, a refund was supposed to be given to a particular address, and if the address is a contract that implements logic requiring more than 2300 gas on receiving Ether, the refund will revert and the user will not receive the refund.
More information: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
Recommended mitigation:
Used call instead. For example:
(bool success, ) = msg.sender.call{amount}(""); require(success, "Transfer failed.");
There are 7 instances of this issue:
FILE: contracts/gas-service/AxelarGasService.sol 128: if (amount > 0) receiver.transfer(amount); 144: receiver.transfer(amount);
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);
FILE: xc20/contracts/XC20Wrapper.sol 63: payable(msg.sender).transfer(address(this).balance);
https://github.com/code-423n4/2022-07-axelar/blob/main/xc20/contracts/XC20Wrapper.sol#L63
Missing zero address check may lead to a user accidentally transferring Ether to the 0 address, resulting in loss of funds.
Recommended mitigation:
Check to ensure the target address is not 0x0 before transfering Ether.
There are 3 instances of this issue:
FILE: contracts/deposit-service/ReceiverImplementaion.sol 23: 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);
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 if required.
There is 1 instance of this issue:
FILE: contracts/deposit-service/AxelarDepositService.sol 241: function contractId() public pure returns (bytes32) {
#0 - re1ro
2022-08-05T05:10:00Z
Dup #4
Dup #3
Good spot
#1 - GalloDaSballo
2022-09-01T00:10:32Z
L
L
NC
#2 - GalloDaSballo
2022-09-01T00:11:10Z
2L 1NC
🌟 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
31.8812 USDC - $31.88
| Issue | Instances |
|---|---|
1. Use ++i instead of i++ to save gas | 4 |
| 2. Cache array length outside of loop | 7 |
3. Use != 0 instead of > 0 for a uint | 7 |
| 4. Let the default value be applied to variables initialized to the default value | 5 |
| 5. Return values directly without an intermediate return variable | 1 |
++i instead of i++ to save gasThis is especially relevant for the use of i++ in for loops. This saves 6 gas per loop.
There are 4 instances of this issue:
FILE: contracts/gas-service/AxelarGasService.sol 123: for (uint256 i; i < tokens.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++) {
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration. To do this, create a variable containing the array length before the loop.
_There are 7 instances of this issue:
FILE: contracts/gas-service/AxelarGasService.sol 123: for (uint256 i; i < tokens.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.auth/AxelarAuthWeighted.sol 17: for (uint256 i; i < recentOperators.length; ++i) { 98: for (uint256 i = 0; i < signatures.length; ++i) { 116: for (uint256 i; i < accounts.length - 1; ++i) {
https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L17
uintSince the integers are unsigned, != 0 and > 0 are equivalent. Using != 0 is 6 gas per instance cheaper than > 0
There are 7 instances of this issue:
FILE: contracts/gas-service/AxelarGasService.sol 128: if (amount > 0) receiver.transfer(amount); 131: if (amount > 0) _safeTransfer(token, receiver, amount);
FILE: contracts/deposit-service/AxelarDepositService.sol 165: if (addressForNativeDeposit(salt, refundAddress, destinationChain, destinationAddress).balance > 0 && msg.sender != refundAddress)
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);
FILE: contracts/auth/AxelarAuthWeighted.sol 76: if (epochForHash[newOperatorsHash] > 0) revert SameOperators();
https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L76
Letting the default value of 0, false be initialized to variables costs less gas compared to initializing it to these default values.
There are 5 instances of this issue:
FILE: contracts/auth/AxelarAuthWeighted.sol 68: uint256 totalWeight = 0; 69: for (uint256 i = 0; i < weightsLength; ++i) { 94: uint256 operatorIndex = 0; 95: uint256 weight = 0; 98: for (uint256 i = 0; i < signatures.length; ++i) {
https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L68
Initializing a return variable for a function, then assigning a value to it requires more gas compared to simply returning the value, as long as the variable is not being used elsewhere in the function.
There is 1 instance of this issue:
FILE: contracts/auth/AxelarAuthWeighted.sol 26: function validateProof(bytes32 messageHash, bytes calldata proof) external view returns (bool currentOperators) {
https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L26
#0 - re1ro
2022-08-05T05:13:19Z
Dup #2
Good spot. Will change to the simple return and add comments of what happens
#1 - GalloDaSballo
2022-08-23T00:47:34Z
Less than 300 gas saved
#2 - GalloDaSballo
2022-08-23T00:50:01Z
Fyi no difference for 5 it's not valid
#3 - GalloDaSballo
2022-08-23T00:50:24Z
See benchmark
<img width="737" alt="Screenshot 2022-08-23 at 02 50 17" src="https://user-images.githubusercontent.com/13383782/186044476-54c42259-7b1d-4137-b782-6e5e3e962aa2.png">// SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.13; import "../../lib/test.sol"; import "../../lib/Console.sol"; contract GasTest is DSTest { DeclaredReturn c0; DirectReturn c1; function setUp() public { c0 = new DeclaredReturn(); c1 = new DirectReturn(); } function testGas() public { c0.doTheThing(); c1.doTheThing(); } } contract DeclaredReturn { function doTheThing() external returns (uint value){ uint value = 123; } } contract DirectReturn { function doTheThing() external returns (uint){ uint value = 123; return value; } }