This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().
2. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
Consider using safeTransfer/safeTransferFrom or require() consistently.
3. The Contract Should Approve(0) first
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.
Approve with a zero amount first before setting the actual amount.
4. Missing checks for approve()’s return status
Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin’s safeApprove(), which reverts if there’s a failure, instead
1. USE SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE
This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().
Instances:
contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64
Recommended Mitigation Steps
Update to safeapprove in the function.
2. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
Instances:
contracts/gas-service/AxelarGasService.sol: L173 L128 L144
contracts/deposit-service/ReceiverImplementation.sol: L23 L51 L71 L86
xc20/contracts/XC20Wrapper.sol: L63 L107
contracts/AxelarGateway.sol: L501 L523
Reference:
This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.
Recommended Mitigation Steps:
Consider using safeTransfer/safeTransferFrom or require() consistently.
3. The Contract Should Approve(0) first
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
IERC20(token).approve(address(operator), 0); IERC20(token).approve(address(operator), amount);
Proof of Concept
contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64
Recommended Mitigation Steps
Approve with a zero amount first before setting the actual amount.
4. Missing checks for approve()’s return status
Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin’s safeApprove(), which reverts if there’s a failure, instead
Instances:
contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64
Reference:
https://code4rena.com/reports/2022-05-sturdy#l-03-missing-checks-for-approves-return-status
5. USE OF FLOATING PRAGMA
Recommend using fixed solidity version
Instances
contracts/interfaces/IDepositBase.sol contracts/interfaces/IAxelarGasService.sol contracts/interfaces/IAxelarAuth.sol contracts/interfaces/IAxelarAuthWeighted.sol contracts/interfaces/IAxelarDepositService.sol
6. Used receive() function will lock Ether in contract
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.
Instances:
contracts/deposit-service/AxelarDepositServiceProxy.sol:13 contracts/deposit-service/DepositReceiver.sol:29