The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug free status. The audit documentation is for discussion purposes only. The information presented in this report is confidential and privileged. If you are reading this report, you agree to keep it confidential, not to copy, disclose or disseminate without the agreement of the Client. If you are not the intended recipient(s) of this document, please note that any disclosure, copying or dissemination of its content is strictly forbidden.
A group of auditors are involved in the work on the audit. The security engineers check the provided source code independently of each other in accordance with the methodology described below:
- Project documentation review.
- General code review.
- Reverse research and study of the project architecture on the source code alone.
- Build an independent view of the project's architecture.
- Identifying logical flaws.
- Manual code check for vulnerabilities listed on the Contractor's internal checklist. The Contractor's checklist is constantly updated based on the analysis of hacks, research, and audit of the clients' codes.
- Code check with the use of static analyzers (i.e Slither, Mythril, etc).
Eliminate typical vulnerabilities (e.g. reentrancy, gas limit, flash loan attacks etc.).
- Detailed study of the project documentation.
- Examination of contracts tests.
- Examination of comments in code.
- Comparison of the desired model obtained during the study with the reversed view obtained during the blind audit.
- Exploits PoC development with the use of such programs as Brownie and Hardhat.
Detect inconsistencies with the desired model.
- Cross check: each auditor reviews the reports of the others.
- Discussion of the issues found by the auditors.
- Issuance of an interim audit report.
- Double-check all the found issues to make sure they are relevant and the determined threat level is correct.
- Provide the Client with an interim report.
- The Client either fixes the issues or provides comments on the issues found by the auditors. Feedback from the Customer must be received on every issue/bug so that the Contractor can assign them a status (either "fixed" or "acknowledged").
- Upon completion of the bug fixing, the auditors double-check each fix and assign it a specific status, providing a proof link to the fix.
- A re-audited report is issued.
- Verify the fixed code version with all the recommendations and its statuses.
- Provide the Client with a re-audited report.
- The Customer deploys the re-audited source code on the mainnet.
- The Contractor verifies the deployed code with the re-audited version and checks them for compliance.
- If the versions of the code match, the Contractor issues a public audit report.
- Conduct the final check of the code deployed on the mainnet.
- Provide the Customer with a public audit report.
All vulnerabilities discovered during the audit are classified based on their potential severity and have the following classification:
| Severity | Description |
|---|---|
| Critical | Bugs leading to assets theft, fund access locking, or any other loss of funds. |
| High | Bugs that can trigger a contract failure. Further recovery is possible only by manual modification of the contract state or replacement. |
| Medium | Bugs that can break the intended contract logic or expose it to DoS attacks, but do not cause direct loss funds. |
| Low | Bugs that do not have a significant immediate impact and could be easily fixed. |
Based on the feedback received from the Customer regarding the list of findings discovered by the Contractor, they are assigned the following statuses:
| Status | Description |
|---|---|
| Fixed | Recommended fixes have been made to the project code and no longer affect its security. |
| Acknowledged | The Customer is aware of the finding. Recommendations for the finding are planned to be resolved in the future. |
The protocol is a participant of the CowSwap solver and allows producing exchanges of tokens in one transaction. Swaps might be various and are joined in batches, executed sequentially.
| Title | Description |
|---|---|
| Client | BarterDAO |
| Project name | Argon |
| Timeline | March 1 2023 - September 01 2023 |
| Number of Auditors | 3 |
| Date | Commit Hash | Note |
|---|---|---|
| 11.02.2023 | 22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00 | Commit for the review |
| 25.02.2023 | 8f4592bbcf1e9328c18632813fef80a68b465a74 | Code with fixes for audit |
| 03.03.2023 | 0d364bc386e48a74ffe6499e13a77fbf81343631 | Code with fixes for re-audit |
| 03.03.2023 | 592bb5fde1579773013e7a54e9842e4fa40572bf | Commit with all fixes |
| 23.08.2023 | 7a16998ceb24237a728b3edf53e35544f06ccf49 | Commit for the diff audit |
| 01.09.2023 | 4278253caa5305518c6d7282688c1e86c7dfc3d0 | Commit for the diff audit 2 |
The audit covered the following files:
| File name | Contract deployed on mainnet | Comment |
|---|---|---|
| SwapExecutor.sol | 0x966899c0d1e00c63aa635a2a19aa0d4ff7744bd6 | Deploy after the diff audit |
| SwapFacade.sol | 0x179dc3fb0f2230094894317f307241a52cdb38aa | Deploy after the diff audit |
| UniswapXBarterReactorCallback.sol | 0x0ca24498339bCB6890EB8A8e216179C06a2D58Cf | Deploy after the diff audit |
| ProtocolHelper.sol | 0x6f9508F7d373f42dF19aBb275eFB50E11892F5dE | |
| SwapGuardV2.sol | 0xA6F0329fA07F7D8b4027e3d162428Dc29D28e8e6 |
| Severity | # of Findings |
|---|---|
| CRITICAL | 0 |
| HIGH | 3 |
| MEDIUM | 5 |
| LOW | 12 |
During the audit process, the developers spotted and acknowledged 3 HIGH, 5 MEDIUM, and 12 LOW severity findings. After working on the reported findings, all of them were acknowledged or fixed by the client.
Not found
SwapExecutor, a contract responsible for executing token swaps, is vulnerable to multiple methods of bypassing payment fees, allowing an attacker to perform swaps without paying the required fees.
The first method involves specifying a personal transfer of all tokens in the final swap, leaving only 2 wei of tokenToTransfer on the contract to avoid paying fees. This enables the attacker to evade payment and execute swaps at no cost.
The second method involves passing a swap-path that ends with a poisonous token created by the attacker. The attacker pays fees in their own token, which is worthless, thus bypassing payment fees.
The third method involves copying the code of SwapExecutor and removing the payment logic, allowing the attacker to continue using the SwapFacade without paying any fees by passing the custom SwapExecutor to SwapFacade.swap().
Overall, these vulnerabilities allow attackers to perform swaps without paying the required fees, leading to potential financial losses for the SwapExecutor owners.
One way to address the vulnerabilities in SwapExecutor is to take a fixed fee in a fixed token, such as Ether, in SwapFacade.
Fixed in commit https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
Client’s commentary: we will remove all fee related logics. MixBytes: contracts do not take fees now, the issue disappeared.
Since we receive HashflowQuote before the call occurred, the information at the time of the transaction may not be up to date.
In that case, due to this code:
if (amount > quote.maxBaseTokenAmount) {
emit AmountExceedsQuote(amount, quote.maxBaseTokenAmount);
quote.effectiveBaseTokenAmount = quote.maxBaseTokenAmount;
} else {
quote.effectiveBaseTokenAmount = amount;
}
part of the money may remain with the SwapExecutor.
We recommend adding a revert if the input amount is not actual.
Acknowledged
Client’s commentary: Won't fix. We are aware that sometimes we can get more than expected amount of funds. Reverting could lead to situation where an 1M trade with slight positive slippage would revert due to 0.01$ surplus to hashflow quote. We do not expect that amount difference will be significant, and we can keep this small difference on contract because tranferring it to user or somewhere else can be an unwanted and surprising behavior. MixBytes: If
minReturnis always actual and the transaction is completed quickly (usingdeadline), then this point can indeed be accepted.
ERC777 tokens allow hooks before and after balance changes.
Transfer _from and to can choose the exact receivers of these hooks via Registry.setInterfaceImplementer():
Also, Executor allows anyone to call executeSwap() and decide on the following targets and calldata.
- An attacker uses
executeSwap()so that Executor callsERC1820Registry.setInterfaceImplementer(implementer=attacker). - User A swaps an ERC777 token to something with low
minReturn. It must be at least one ERC777 among the list of tokens in swaps. - Executor receives ERC777 tokens from Facade (hook).
- The attacker reverts on the hook.
As a result, any ERC777 token going through Executor will lead to DoS and swap will revert.
So, one of the attack flows can be like this:
- Attacker uses
executeSwap()so that Executor calls `ERC1820Registry.setInterfaceImplementer(implementer=attacker)'. So, anytime Executor receives ERC777 tokens, a frontrunner contract will be called. - User A swaps the ERC777 tokens to something with low
minReturn. - Executor receives the ERC777 tokens from Facade (hook).
- Attacker contract enters Executor.executeSwap() and Executor thinks that current ERC777 tokens belong to Attacker. So, he can withdraw these tokens, or swap them to something.
Or at least Attacker can revert when recieving a call, not allowing a swap with this token at all.
We recommend that Facade be the only allowed caller for SwapExecutor.execureSwap() with the Reentrancy guard and be sure that minReturn is set even for small swaps.
Fixed in commit https://github.com/BarterLab/argon/commit/0d364bc386e48a74ffe6499e13a77fbf81343631
Client’s commentary: Fixed MixBytes: Fixed as SwapExecutor now reverts on call to the
0x1820...aD24address, so it is not allowed to set a malicious contract as an interface implementer.
SwapFacade allows to choose any SwapExecutor address, even not authored by the team. Fees are charged on SwapExecutor. So, it is rational to fork SwapExecutor with zero fees and choose it as an executor for swaps.
Some other possible scenarios:
- users can choose out date executors
- users can choose executors with bugs
We recommend having a list of allowed executors and taking fees on SwapFacade.
Acknowledged
Client’s commentary: Having fees on SwapFacade won't help either because it can be forked as well as SwapExecutor. Therefore, we remove all fee-related logics as inefficient and gas-consuming.
Funds left on the SwapExecutor can be withdrawn by anyone who specifies a transfer call in the swaps.
There are multiple reasons why funds may be left on the SwapExecutor:
- A user may not set the
tokenRatioto 100% in a swap.
uint256 poolSwapAmount = (balanceToSwap * swap.tokenRatio) / _ONE;
# sum (tokenRatio) > _ONE - is not risky, revert happens
# sum (tokenRatio) < _ONE - is risky, as some tokens
are left not swapped and can be taken by anyone.
- The
SwapExecutormay run arbitrary calls to protocols that reward it with additional tokens. TheSwapFacadeonly checks thetokenToTransfer, and a user may forget to transfer other tokens from theSwapExecutorimmediately. - If a user transfers tokens to the
SwapExecutorfirst and then runs theSwapFacade.swap()function in a separate transaction.
To address this, a similar approach to that used in CowProtocol could be implemented: allowing only whitelisted managers to execute swaps in SwapExecutor.
It is also recommended to check that all tokenRatio used eventually sum up to 100%.
Acknowledged
Client’s commentary: Won't fix. We are aware of this behavior and we have external checks on side that forms calldata to prevent this from happening. It is also possible to use this as part of bigger flow, i.e. let's consider for example integration with some service that expects to be paid 1% fee otherweise transaction will revert. We are able to make last step transferring 1% of tokens to some address and this is an entire step. It doesn't sum to 1 as other steps but it's a valid use case we want to support.
SwapExecutor has the following lines for ETH:
if (value > valueLimit) {
value = valueLimit;
}
(success, result) = target.call{ value: value }(data);
So, only a limited amount will be used if sent ETH is above valueLimit. This ETH will be lost on the contract and anyone can withdraw these exceeded amounts.
We recommend redesigning valueLimit purpose so that no funds are lost in edge situations.
Acknowledged
Client’s commentary: Won't fix. This is an ETH special case for 2.2.2 (Hashflow) or more generaly speaking for any quoting mechanism. We expect this difference to only exist on market movements that can't be big enough (deadline check is in place in order to guarantee this). So the same logics as 2.2.2 applies here except it's about ETH not ERC20 tokens.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
Facade does not check that msg.sender is the owner of a permit. So, a frontrunner can use permits of other users available from mempool, so that their transactions would revert (as permit are used by the frontrunner).
A user will receive revert and will have to build new calldata, without permit.
We recommend decoding permits to extract their owner, then check that msg.sender is this owner. Some example used by 1inch:
SwapGuardV2 functions are planned to be used as separate calls in a set of swaps. However, if during the swaps process a hacker manages to get a callback to their own contract (for example, if the victim operates with ERC-777 tokens), the hacker can call public accessible function makeCheckpoint() to modify the SwapGuardV2 state so that the ensureCheckpoint() function at the victim's end would not work correctly.
It is recommended to revise the approach to using SwapGuardV2. It may be reasonable to leave only one function in SwapGuardV2, which receives a set of tokens, deltas and a callback. The function records the current token balances in memory instead of a storage, then calls the user's callback (which performs swaps), and then checks for changes in the balances.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
Client’s commentary: We will add msg.sender check so only the person who initially called
makeCheckountwill be able toensureCheckpoint.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
SwapExecutor uses the Ownable functionality:
The Ownable contract can be upgraded to Open Zeppelin's Ownable2Step:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol
Ownable2Step provides added safety due to its securely designed two-step process.
We recommend applying Ownable2Step instead of Ownable.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
The current implementation manually packs and unpacks the fee and address from a uint256 value, which increases the likelihood of errors and may cause unnecessary gas usage.
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/SwapExecutor.sol#L48
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/SwapExecutor.sol#L137
To simplify the logic and optimize gas usage, it's recommended to store the fee and address separately in the contract's state using two separate variables:
uint160 feeAddress;
uint96 fee;
Both variables will fit into a single storage slot. This will also allow more efficient retrieval of the fee-related data.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
There are no zero checks for recipient and minReturn:
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/SwapFacade.sol#L32
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/SwapFacade.sol#L33
Some programs may pass zero values for some arguments by default. A user may not notice this behaviour and lose funds.
It is recommended to add requirements which check that the recipient and minReturn are not zero.
- Contracts that use SwapGuardV2 together with Facade+Executor are out of scope
- All functions are public and anyone can makeCheckpoint()
- This contract will not work with native ETH.
- allowedLoss adds up with tokens having different decimals. Or tokensPrices must at least include decimals
- Lengths of tokens, tokenPrices and balanceChanges are not checked to be the same
- All checkpoints are written to storage, no need if it is used in one transaction
We recommend using more efficient ways to check profitability of swaps.
Acknowledged
MixBytes: Points 2 and 3 are fixed, the other points are acknowledged. After an internal discussion with the customer, we concluded that the status is Acknowledged.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
Both Facade and Executor inherit ContractOnlyEthRecipient.sol with the following code.
/**
* @title ContractOnlyEthRecipient
* @notice Base contract that rejects any direct ethereum deposits.
This is a failsafe against users who can accidentaly send ether
*/
abstract contract ContractOnlyEthRecipient {
receive() external payable {
// solhint-disable-next-line avoid-tx-origin
if (msg.sender == tx.origin) {
revert DirectEthDepositIsForbidden();
}
}
}
In fact, only Executor needs this inheritance, as it can receive ETH from external exchanges. But Facade should only receive ETH as msg.value at swap().
Also, in the comments developers stated that it is against accidential ETH sendings. By the way, it still allows to receive accidential ETH from users as contracts (like user wallets). Moreover, any accidential ETH on balances are bad (can be stolen), so it is better to remove options to receive accidential money when it’s possible.
We recommend removing recieve() for SwapFacade and SwapGuardV2.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
depositETH() and withdrawETH() are inherited and make calls to WETH.
It is used to swap between native ETH and WETH. But the implementation makes Executor call itself (external call), send ETH itself.
This step is useless and can be dropped.
function depositWeth(uint256 amount) external payable {
if (amount != msg.value) {
revert EthValueAmountMismatch();
}
weth.deposit{value: amount}();
}
function withdrawWeth(uint256 amount) external {
weth.withdraw(amount);
}
We recommend removing these functions and use WETH as target directly.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
abi.encodeCall is a safer way to avoid mistakes during compilation. The compiler checks that the types of args are compatible with a call.
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/helpers/UniswapV2Helper.sol#L31
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/helpers/HashflowHelper.sol#L30
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/helpers/DssPsmHelper.sol#L31
- https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/helpers/DodoV1Helper.sol#L22
We recommend changing encodeWithSelector to encodeCall.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
The uniswapV3SwapCallback() function in the IUniswapV3SwapCallback interface, which is part of the Uniswap v3 protocol, is used to handle the results of a swap. However, the function in UniswapV3Executor does not include any caller verification, which can lead to potential vulnerabilities.
The uniswap team recommends checking that the caller of the uniswapV3SwapCallback() is a UniswapV3Pool deployed by the canonical UniswapV3Factory:
Note that this increases the amount of gas used.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
ERC-20 token funds cannot be withdrawn from SwapFacade if a user accidentally sends tokens to the SwapFacade contract instead of SwapExecutor.
It is recommended to add a sweep(token) onlyOwner function with the onlyOwner modifier. This function will allow the owner of the contract to sweep any funds that are stuck in the contract and transfer them to a designated account. This will provide a safety net for users who accidentally send funds to the contract, preventing their funds from being lost or stolen.
In SwapExecutor this line uint256 poolSwapAmount = (balanceToSwap * swap.tokenRatio) / _ONE allows making overflow.
uint256 poolSwapAmount = (balanceToSwap * swap.tokenRatio) / _ONE;
We recommend removing unsafe math from critical functions.
Acknowledged
Client's commentary: Won't fix. We would like to reenable checked math for this multiplication but solidity provide no such tools. After considering possible fixes we decided on touchin this for following reasons: overflow would reult in drastical reduction of swap input on the step, which will lead to great reduction in output token. This should be handled by minReturn or other slippage tolerance techniques. Moreover, overflow can only happen if left argument is greater than 2^(256-18), since 2^18 is a limit for tokenRatio. Considering most of tokens have total supply around 2^30 it looks unrealistic to hit these bounds.
UniswapV2Helper duplicates the existing code of UniswapV2Router from the official Uniswap repo.
We recommend using the existing functionality to avoid potential mistakes.
Acknowledged
Client's commentary: Won't fix. We would like to not pay for external call, we checked that our implementation is identical and wish to stick with it. MixBytes: Calling
swapUniswapV2in Protocol Helper requires an external call too. There is no error here, so the status is Acknowledged.
Fixed in https://github.com/BarterLab/argon/commit/8f4592bbcf1e9328c18632813fef80a68b465a74
The logic in SwapFacade.swap() can be simplified to always call the _permit().
The logic at lines 59-73 https://github.com/BarterLab/argon/blob/22239aaccdbb78d4aa7ac5c4d0859a9b31c0fc00/contracts/SwapFacade.sol#L63
uint256 currentBalance = sourceToken.balanceOf(address(executor));
if (currentBalance < amount)
{
if (permit.length > 0) {
_permit(address(sourceToken), permit);
}
uint256 approveAmount = sourceToken.allowance(
msg.sender,
address(this)
);
if (approveAmount < amount) {
revert NotEnoughApprovedFundsForSwap(approveAmount, amount);
}
sourceToken.safeTransferFrom(msg.sender, address(executor), amount);
}can be simplified down to just two lines (always call _permit()):
_permit(address(sourceToken), permit);
sourceToken.safeTransferFrom(msg.sender, address(executor), amount);Note that the 1inch _permit() is already checking permit.length and does nothing in case it is zero:
contract Permitable {
error BadPermitLength();
function _permit(address token, bytes calldata permit)
internal virtual {
if (permit.length > 0) {
bool success;
if (permit.length == 32 * 7) {
// solhint-disable-next-line avoid-low-level-calls
(success,) = token.call(
abi.encodePacked(IERC20Permit.permit.selector, permit)
);
} else if (permit.length == 32 * 8) {
// solhint-disable-next-line avoid-low-level-calls
(success,) = token.call(
abi.encodePacked(IDaiLikePermit.permit.selector, permit)
);
} else {
revert BadPermitLength();
}
if (!success) {
RevertReasonForwarder.reRevert();
}
}
}
}It is recommended to remove unnecessary conditions to save gas.
The project contains smart contracts that require active monitoring. For these purposes, it is recommended to proceed with developing new monitoring events based on Forta (https://forta.org) with which you can track the following exemplary incidents:
- Using an unexpected
Executorduring swap - Either ERC20 tokens or ETH remain on the balance of contracts (SwapExecutor, SwapFacade)
- A wrong
minReturnordeadlineare passed
MixBytes is a team of blockchain developers, auditors and analysts keen on decentralized systems. We build opensource solutions, smart contracts and blockchain protocols, perform security audits, work on benchmarking and software testing solutions, do research and tech consultancy.