Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Missing Check for Zero Address ParaSwapUtils library

mibrobuba

Summary

The swap function in the ParaSwapUtils library does not include a check to ensure that the fromToken address is not the zero address (address(0)). This could lead to unexpected behavior or transaction failures when interacting with the function.

Vulnerability Details

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.4;
​
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
​
interface IAugustusSwapper {
function getTokenTransferProxy() external view returns (address);
}
​
​
library ParaSwapUtils {
using SafeERC20 for IERC20;
​
​
function swap(address to, bytes memory callData) external {
_validateCallData(to, callData);
address approvalAddress = IAugustusSwapper(to).getTokenTransferProxy();
address fromToken;
uint256 fromAmount;
assembly {
fromToken := mload(add(callData, 68))
fromAmount := mload(add(callData, 100))
}
IERC20(fromToken).safeApprove(approvalAddress, fromAmount);
(bool success, ) = to.call(callData);
require(success, "paraswap call reverted");
}
Proof Of Concept

Setup

  1. Deploy the ParaSwapUtils library.

  2. Craft malicious callData where fromToken is set to address(0).

  3. Call the swap function with the malicious callData.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.4;
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
interface IAugustusSwapper {
function getTokenTransferProxy() external view returns (address);
}
​
library ParaSwapUtils {
using SafeERC20 for IERC20;
​
function swap(address to, bytes memory callData) external {
_validateCallData(to, callData);
address approvalAddress = IAugustusSwapper(to).getTokenTransferProxy();
address fromToken;
uint256 fromAmount;
assembly {
fromToken := mload(add(callData, 68))
fromAmount := mload(add(callData, 100))
}
IERC20(fromToken).safeApprove(approvalAddress, fromAmount);
(bool success, ) = to.call(callData);
require(success, "paraswap call reverted");
}
​
function _validateCallData(address to, bytes memory callData) internal view {
require(to == address(0xDEF171Fe48CF0115B1d80b88dc8eAB59176FEe57), "invalid paraswap callee");
address receiver;
assembly {
receiver := mload(add(callData, 196))
}
require(receiver == address(this), "invalid paraswap calldata");
}
}
​
// Malicious Contract to Demonstrate the Issue
contract MaliciousCaller {
function exploitZeroAddressBug(address paraSwapUtilsAddress) external {
// Craft malicious callData with fromToken = address(0)
bytes memory maliciousCallData = abi.encodeWithSignature(
"swap(address,bytes)",
address(0xDEF171Fe48CF0115B1d80b88dc8eAB59176FEe57), // Valid AugustusSwapper address
abi.encode(
address(0), // fromToken = address(0)
100, // fromAmount = 100
address(this) // receiver = address(this)
)
);
​
// Call the swap function with malicious callData
(bool success, ) = paraSwapUtilsAddress.call(maliciousCallData);
require(success, "Exploit failed");
}
}

Steps to Reproduce

  • Deploy the ParaSwapUtils library to a testnet or local blockchain.

  • Deploy the MaliciousCaller contract, which will be used to demonstrate the bug.

  • Call the exploitZeroAddressBug function in the MaliciousCaller contract, passing the address of the deployed ParaSwapUtils library.

  • The transaction will revert with an error like "SafeERC20: approve from non-zero to non-zero allowance" because the safeApprove function is called with fromToken = address(0).

Expected Output

  • The transaction will fail because safeApprove is called with fromToken = address(0), which is not a valid ERC20 token address.

  • The error message will not explicitly indicate that the issue is due to a zero address, making it difficult for users to diagnose the problem.

Impact

  • fromToken is address(0), the safeApprove call will revert, causing the entire transaction to fail.

  • The error message will not explicitly indicate that the issue is due to a zero address, making it difficult for users to diagnose the problem.

  • If the callData is malformed or manipulated, the function could inadvertently attempt to interact with the zero address, resulting in unintended behavior.

Tools Used

Manual Code Review

Recommendations

add a check to ensure fromToken is not the zero address

require(fromToken != address(0), "Invalid token address: zero address not allowed");
Updates

Lead Judging Commences

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Users mistake, only impacting themselves.

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Users mistake, only impacting themselves.

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.