Conversation
contracts/Interfaces.sol
Outdated
| function mint() external payable; | ||
|
|
||
| function repayBorrowBehalf(address borrower) external payable; | ||
|
|
||
| function liquidateBorrow(address borrower, IVToken vTokenCollateral) external payable; |
There was a problem hiding this comment.
Please include explicit return types in the function signatures.
There was a problem hiding this comment.
VBNB doesn't have return types in the following functions.
contracts/SwapRouter/SwapRouter.sol
Outdated
| IComptroller public immutable COMPTROLLER; | ||
|
|
||
| /// @notice The swap helper contract for executing token swaps | ||
| SwapHelper public immutable swapHelper; | ||
|
|
||
| /// @notice The wrapped native token contract (e.g., WBNB) | ||
| IWBNB public immutable wrappedNative; |
There was a problem hiding this comment.
For consistency, could we use capitalized naming for all immutable variables?
contracts/SwapRouter/SwapRouter.sol
Outdated
| event SweepNative(address indexed receiver, uint256 amount); | ||
|
|
||
| /// @custom:error Thrown when an invalid vToken address is provided | ||
| error InvalidVToken(address vToken); |
There was a problem hiding this comment.
InvalidVToken doesn’t seem to be used anywhere, we can remove it
contracts/SwapRouter/SwapRouter.sol
Outdated
| IWBNB public immutable wrappedNative; | ||
|
|
||
| /// @notice The native vToken address (e.g., vBNB) | ||
| address public immutable NATIVE_VTOKEN; |
There was a problem hiding this comment.
For consistency with other immutable contract references, can we use the IVBNB interface here as well?
contracts/SwapRouter/SwapRouter.sol
Outdated
| if (vToken == NATIVE_VTOKEN) { | ||
| return address(wrappedNative); |
There was a problem hiding this comment.
I understand we’re returning the wrapped asset here instead of the native one since the native token doesn’t have an address and it simplifies the swap logic. This is later handled in _supply().
However, should we continue supporting this mapping for the native market? Since we plan to deprecate the native market and move fully to the wrapped version, should we all together skip the vBNB minting wdyt?
There was a problem hiding this comment.
I agree with this approach, and it's worth discussing further.
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 balanceBefore = IERC20Upgradeable(tokenOut).balanceOf(address(this)); | ||
|
|
||
| // Execute swap using SwapHelper multicall | ||
| bytes memory swapCallData = abi.encodeWithSelector(swapHelper.multicall.selector, swapData); |
There was a problem hiding this comment.
Please update swapHelper, the multicall interface has changed, so the encoding here should be adjusted accordingly.
contracts/SwapRouter/SwapRouter.sol
Outdated
| * @param amount The amount to supply | ||
| * @return amountSupplied The actual amount supplied | ||
| */ | ||
| function _supply(address vToken, address token, uint256 amount) internal returns (uint256 amountSupplied) { |
There was a problem hiding this comment.
Can we rename the token parameter to asset or underlyingToken? for clarity.
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 vTokenBalance = IERC20Upgradeable(vToken).balanceOf(address(this)); | ||
| if (vTokenBalance > 0) { | ||
| IERC20Upgradeable(vToken).safeTransfer(msg.sender, vTokenBalance); |
There was a problem hiding this comment.
If we decide to support vBNB minting, I would prefer to move this transfer block probably inside the NATIVE_VTOKEN specific if block.
There was a problem hiding this comment.
Can you clarify why we should move it inside the if block?
There was a problem hiding this comment.
agree, why not move this into NATIVE_VTOKEN block ?
The reason for this is only NATIVE_VTOKEN doesn't support mintBehalf.
contracts/SwapRouter/SwapRouter.sol
Outdated
| * @return actualAmountIn The actual amount transferred | ||
| */ | ||
| function _handleTokenInput(address tokenIn, uint256 amountIn) internal returns (uint256 actualAmountIn) { | ||
| if (tokenIn == address(0)) { |
There was a problem hiding this comment.
Instead of mapping the native token to address(0), I’d prefer using the NATIVE_TOKEN_ADDR constant (0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB) for consistency with how we handle native tokens in other contracts.
contracts/SwapRouter/SwapRouter.sol
Outdated
| if (vToken == NATIVE_VTOKEN) { | ||
| // Handle native token repayment | ||
| IWBNB(token).withdraw(repayAmount); | ||
| IVBNB(vToken).repayBorrowBehalf{ value: repayAmount }(msg.sender); | ||
| amountRepaid = repayAmount; | ||
| } else { |
There was a problem hiding this comment.
I would prefer this design:
-
Do not allow supplying collateral or repaying for the native market (vBNB).
As we are deprecating the native market. -
It is still fine to accept native BNB as tokenIn.
The contract can wrap the native BNB, use it as input for swaps, and then supply/repay any non-native market.
Native BNB should only act as input to the flow not as the target market itself.
There was a problem hiding this comment.
i think it's fine if we support it, it doesn't bring any drawback or complicate the logic a lot
contracts/SwapRouter/SwapRouter.sol
Outdated
| IERC20Upgradeable(token).safeApprove(vToken, 0); | ||
| IERC20Upgradeable(token).safeApprove(vToken, amount); |
There was a problem hiding this comment.
safeApprove is deprecated in oz v4 and removed in oz v5. We could use forceApprove or approveOrRevert
contracts/SwapRouter/SwapRouter.sol
Outdated
| if (amount > repayAmount) { | ||
| uint256 excess = amount - repayAmount; | ||
| if (vToken == NATIVE_VTOKEN) { | ||
| // Convert back to wrapped native and transfer | ||
| wrappedNative.deposit{ value: excess }(); | ||
| IERC20Upgradeable(address(wrappedNative)).safeTransfer(msg.sender, excess); |
There was a problem hiding this comment.
In _repay, we only unwrap repayAmount, not the full amount.
So the excess stays as WBNB, not native BNB.
But the code tries to re-wrap it using:
wrappedNative.deposit{ value: excess }();
This might fails because there’s no native BNB corresponding to that excess.
We should just transfer the excess WBNB back to the user instead.
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 amountIn, | ||
| uint256 minAmountOut, | ||
| bytes[] calldata swapData | ||
| ) external payable nonReentrant { |
There was a problem hiding this comment.
Any specific reason for making swapAndSupply and swapAndRepay payable when native handling already uses separate functions (swapNativeAndSupply and swapNativeAndRepay)?
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 debtAmount = IVToken(vToken).borrowBalanceCurrent(msg.sender); | ||
| if (debtAmount == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
I would prefer adding this as an early-revert check in all external repay-related functions instead of here, similar to how it’s already done in swapAndRepayFull
contracts/SwapRouter/SwapRouter.sol
Outdated
| // Return any excess tokens to user | ||
| if (amountOut > debtAmount) { | ||
| uint256 excess = amountOut - debtAmount; | ||
| IERC20Upgradeable(tokenOut).safeTransfer(msg.sender, excess); | ||
| } |
There was a problem hiding this comment.
Nit: Can we refactor the logic so excess tokens are transferred back to the user in one place instead of doing it twice, once here and once inside _repay?
I understand the calculations differ, but we can still consolidate the return excess logic to avoid duplication and make the flow cleaner.
| * @dev For native vToken (vBNB), returns wrapped native token address | ||
| */ | ||
| function _getUnderlyingToken(address vToken) internal view returns (address underlying) { | ||
| if (vToken == address(NATIVE_VTOKEN)) { |
There was a problem hiding this comment.
so what's the value of IVToken(vWBNB).underlying() -> WETH ?
There was a problem hiding this comment.
It will be WBNB. However, for VBNB, we are also returning WBNB.
contracts/SwapRouter/SwapRouter.sol
Outdated
| amountSupplied = amount; | ||
| } else { | ||
| // Handle ERC20 token supply | ||
| IERC20Upgradeable(token).forceApprove(vToken, amount); |
There was a problem hiding this comment.
should we reset the approve to 0 after mintBehalf ?
There was a problem hiding this comment.
It's not needed.
contracts/SwapRouter/ISwapRouter.sol
Outdated
| address tokenIn, | ||
| uint256 amountIn, | ||
| uint256 minAmountOut, | ||
| bytes[] calldata swapData |
There was a problem hiding this comment.
should be " bytes calldata swapData " ?
contracts/SwapRouter/SwapRouter.sol
Outdated
| /** | ||
| * @notice Accepts native tokens sent to this contract | ||
| */ | ||
| receive() external payable {} |
There was a problem hiding this comment.
lets add limit so that only wBNB can send fund, wdyt ?
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 amountIn, | ||
| uint256 minAmountOut, | ||
| bytes calldata swapCallData | ||
| ) external payable nonReentrant { |
There was a problem hiding this comment.
can remove payable here
contracts/SwapRouter/SwapRouter.sol
Outdated
| uint256 actualAmountIn = _handleTokenInput(tokenIn, maxAmountIn); | ||
|
|
||
| // Perform swap - no minAmountOut since we need exact debt amount | ||
| uint256 amountOut = _performSwap(tokenIn, tokenOut, actualAmountIn, 0, swapCallData); |
There was a problem hiding this comment.
even tho we should still pass in debtAmount here, i mean we cant guarantee the underlying swap will return exact debtAmount
There was a problem hiding this comment.
The function validates post-swap that amountOut >= debtAmount, which is more practical than constraining the swap upfront - this allows natural slippage while ensuring sufficient funds for full debt repayment.
| uint256 minAmountOut, | ||
| bytes calldata swapCallData | ||
| ) external payable nonReentrant { | ||
| if (msg.value == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
Since this function uses mintBehalf it might be a good idea to check if msg.sender delegated this contract.
There was a problem hiding this comment.
mintBehalf function doesn't require delegation logic.
| uint256 minAmountOut, | ||
| bytes calldata swapCallData | ||
| ) external nonReentrant { | ||
| if (amountIn == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
Since this function uses mintBehalf it might be a good idea to check if msg.sender delegated this contract.
| uint256 minAmountOut, | ||
| bytes calldata swapCallData | ||
| ) external payable nonReentrant { | ||
| if (msg.value == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
Since this function uses mintBehalf it might be a good idea to check if msg.sender delegated this contract.
| uint256 maxAmountIn, | ||
| bytes calldata swapCallData | ||
| ) external payable nonReentrant { | ||
| if (maxAmountIn == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
Since this function uses mintBehalf it might be a good idea to check if msg.sender delegated this contract.
contracts/SwapRouter/SwapRouter.sol
Outdated
| NATIVE_VTOKEN.mint{ value: amount }(); | ||
| amountSupplied = amount; | ||
|
|
||
| // Transfer vBNB tokens to user (only needed for NATIVE_VTOKEN since it doesn't support mintBehalf) |
There was a problem hiding this comment.
Do I understand it correctly: mintBehalf like behaviour on vBNB market is to mint for a contract and transfer vTokens to user ?
There was a problem hiding this comment.
Yes, VBNB doesn't have any mintBehalf logic.
|
overall lgtm, no other major issue found |
Swap Router - merge develop to update SwapHelper
[VPD-458]: Cyfrin Audit Fixes for SwapRouter
[VPD-440]: Pashov Audit Fixes for SwapRouter
Prevents duplicate artifact errors (HH701) by making Interfaces.sol a re-export file pointing to contracts/Interfaces/*.sol, preserving the import path used by SwapRouter without requiring redeployment.
|
## 1.2.0-dev.3 (2026-02-24) * Merge branch 'develop' into feat/swapRouter ([ec44ad8](ec44ad8)) * Merge branch 'develop' into feat/swapRouter-develop-merge ([e233965](e233965)) * Merge branch 'feat/swapper' into feat/swapRouter ([85f2930](85f2930)) * Merge pull request #16 from VenusProtocol/feat/swapRouter ([c4a7e6b](c4a7e6b)), closes [#16](#16) * Merge pull request #31 from VenusProtocol/feat/swapRouter-develop-merge ([059cd0a](059cd0a)), closes [#31](#31) * Merge pull request #43 from VenusProtocol/fix/vpd-440 ([6b3774d](6b3774d)), closes [#43](#43) * Merge pull request #45 from VenusProtocol/fix/VPD-458 ([f949020](f949020)), closes [#45](#45) * chore: clean up .gitignore duplicates and organize sections ([65165ef](65165ef)) * feat: add backend signature verification to the swapper ([e2312fa](e2312fa)) * feat: add barrel Interfaces.sol to re-export split interface files ([1db1685](1db1685)) * feat: add deployment script ([82082ba](82082ba)) * feat: add deployments ([d1d28c0](d1d28c0)) * feat: add generic swapper contract ([642375a](642375a)) * feat: add swapNativeAndRepayFull function ([4487e12](4487e12)) * feat: add SwapRouter audits ([50a4993](50a4993)) * feat: add swapRouter bscmainnet deployment ([9f06257](9f06257)) * feat: add SwapRouter contract ([2981707](2981707)) * feat: add SwapRouter fork test ([3417be5](3417be5)) * feat: I-01 ([0f8abbe](0f8abbe)) * feat: L-01 ([594b4ff](594b4ff)) * feat: L-05 ([aeb20e1](aeb20e1)) * feat: updating deployment files ([8de1ccf](8de1ccf)) * feat: updating deployment files ([41bb9e0](41bb9e0)) * feat: updating deployment files ([de928dd](de928dd)) * refactor: consolidate slippage check in repayFull ([f042ecd](f042ecd)) * test: add unit test for SwapRouter ([fbc3044](fbc3044)) * test: update swapRouter fork tests ([e6add02](e6add02)) * test: update unit test for swapRouter ([215a735](215a735)) * fix: fix comments ([f487c79](f487c79)) * fix: fix unit and fork test ([e21d01b](e21d01b)) * fix: major fixes ([f2459c8](f2459c8)) * fix: tests ([a5bf806](a5bf806))
This PR introduces the
SwapRoutercontract to the Venus periphery, along with its interface, deployment script, and supporting tests. The contract is now deployed and verified on bothbsctestnetandbscmainnet.What’s Included
SwapRouterContractISwapRouterInterfaceDeployment ScriptTestsNetwork Deploymentsbsctestnetandbscmainnet