Axelar Network v2 contest - apostle0x01's results

[L-01] Unused receive() function will lock Ether in contract

Impact

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

deposit-service/DepositReceiver.sol:29:    receive() external payable {}
deposit-service/AxelarDepositServiceProxy.sol:13:    receive() external payable override {}

https://github.com/code-423n4/2022-07-axelar/tree/main/contracts/deposit-service/DepositReceiver.sol#L29 https://github.com/code-423n4/2022-07-axelar/tree/main/contracts/deposit-service/AxelarDepositServiceProxy.sol#L13

[L-02] approve is subject to a known front-running attack. Consider using safeApprove instead:

approve is subject to a known front-running attack. Consider using safeApprove instead:

deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);
deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);

Update to _token.safeApprove(spender, type(uint256).max) in the function.

[G-01] Cache storage values in memory

Impact

Anytime you are reading from storage more than once, it is cheaper to cache variables in memory. An SLOAD cost 100 GAS while MLOAD and MSTORE only cost 3 GAS.

This is especially true in for loops when using the length of a storage array as the condition being checked after each loop. Caching the array length in memory can yield significant gas savings when the array length is high.

// Before
for(uint 1; i < approvals.length; i++); // approvals is a storage variable

// After
uint memory numApprovals = approvals.length;
for(uint 1; i < numApprovals; i++);

POC

auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {
deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {

[G-02] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

auth/AxelarAuthWeighted.sol:69:        for (uint256 i = 0; i < weightsLength; ++i) {
auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

Mitigation

As an example: for (uint256 i = 0; i < weightsLength;) { should be replaced with for (uint256 i; i < weightLength;) {

[G-03] Increments can be unchecked

Impact

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Instances include:

deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

The code would go from:

for (uint256 i; i < refundTokens.length; i++) {  
 // ...  
}  

to:

for (uint256 i; i < refundTokens.length;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

[G-04] ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integers. This is because the pre-increment operation is cheaper (about 5 GAS per iteration). This is implemented in other sol files but looks like its missing from Vault.sol .

POC

deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {