search

code-423n4 / 2022-07-axelar-findings

0 stars 0 forks source link

QA Report #217

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA REPORT

Number of Issues: 2

1. ERC20 return values aren't checked in some places

Impact

The ERC20.transferFrom(), ERC20.transfer(), ERC20.approve() functions return a boolean value indicating success. This parameter needs to be checked for success. But in the code below ,it is not checked at all .

One should use safeTransfer instead of transfer only. As there are popular tokens, such as USDT that transfer/transferFrom method does'nt return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must check the transferFrom return value

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L23

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L38

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

  2022-07-axelar/contracts/deposit-service/AxelarDepositService.sol::30 => IERC20(wrappedTokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::38 => IERC20(tokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::64 => IERC20(wrappedTokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::86 => recipient.transfer(amount);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::128 => if (amount > 0) receiver.transfer(amount);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::144 => receiver.transfer(amount);
  2022-07-axelar/contracts/test/TestWeth.sol::23 => payable(msg.sender).transfer(amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapExecutable.sol::28 => IERC20(tokenA).approve(address(swapper), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapExecutable.sol::31 => IERC20(tokenB).approve(address(gateway), convertedAmount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapForecallable.sol::28 => IERC20(tokenA).approve(address(swapper), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapForecallable.sol::31 => IERC20(tokenB).approve(address(gateway), convertedAmount);
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::24 => IERC20(tokenAddress).transferFrom(msg.sender, address(this), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::36 => IERC20(toTokenAddress).transfer(recipient, convertedAmount);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::37 => IERC20(tokenX).transferFrom(msg.sender, address(this), amount + gasFee);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::39 => IERC20(tokenX).approve(address(gasService), gasFee);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::52 => IERC20(tokenX).approve(address(gateway), amount);
  2022-07-axelar/xc20/contracts/XC20Wrapper.sol::63 => payable(msg.sender).transfer(address(this).balance);

Mitigation

We still recommend checking the return value as a best practice. Consider using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant staking tokens.

References

https://github.com/code-423n4/2021-12-nftx-findings/issues/141

https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

2. NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

Impact

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.

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/interfaces/IAxelarAuth.sol#L3

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/interfaces/IAxelarAuthWeighted.sol#L3

  2022-07-axelar/contracts/interfaces/IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarExecutable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarForecallable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarGateway.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IBurnableMintableCappedERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IDepositBase.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20Burn.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20BurnFrom.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20Permit.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IMintableCappedERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IOwnable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IReceiverImplementation.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/ITokenDeployer.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IUpgradable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IWETH9.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/xc20/contracts/interfaces/LocalAsset.sol::3 => pragma solidity ^0.8.0;
  2022-07-axelar/xc20/contracts/interfaces/Permit.sol::3 => pragma solidity ^0.8.0;

Mitigation

Used a fixed compiler version

References

https://code4rena.com/reports/2022-06-nibbl/#n-16-non-libraryinterface-files-should-use-fixed-compiler-versions-not-floating-ones

GalloDaSballo commented 2 years ago

2. NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

NC

1. ERC20 return values aren't checked in some places

L

1L 1NC