Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions contracts/script/DeployStrategy.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import "forge-std/Script.sol";

import {SolvBTCJUPStrategy, ISolvBTCRouterV2, ISolvBTCRouterV1} from "../src/gateway/strategy/SolvStrategy.sol";
import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";

contract DeployStrategyScript is Script {
address public deployer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The state variable deployer is declared but never used. It should be removed to improve code clarity and reduce contract size.


function run() public {
uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY");

vm.startBroadcast(deployerPrivateKey);

ISolvBTCRouterV2 solvBTCRouterV2 = ISolvBTCRouterV2(0x56a4d805d7A292f03Ead5Be31E0fFB8f7d0E3B48);
ISolvBTCRouterV1 solvBTCRouterV1 = ISolvBTCRouterV1(0xbA46FcC16B464D9787314167bDD9f1Ce28405bA1);
IERC20 solvBTC = IERC20(0x541FD749419CA806a8bc7da8ac23D346f2dF8B77);
IERC20 solvBTCJUP = IERC20(0x6b062AA7F5FC52b530Cb13967aE2E6bc0D8Dd3E4);
bytes32 poolId = 0x6f113a39a50769de40d4f2e7e46b6a4c6d7774e2c3943ced2dbcb25e626d1d04;

new SolvBTCJUPStrategy(solvBTCRouterV1, solvBTCRouterV2, poolId, solvBTC, solvBTCJUP);

vm.stopBroadcast();
}
Comment on lines +12 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This deployment script only deploys the SolvBTCJUPStrategy. However, the PR introduces two new strategies (SolvBTCJUPStrategy and SolvBTCPlusStrategy). For completeness and to match what seems to be intended (based on broadcast JSON files), this script should also deploy SolvBTCPlusStrategy.

}
93 changes: 93 additions & 0 deletions contracts/src/gateway/strategy/SolvStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ import {Context} from "@openzeppelin/contracts/utils/Context.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

/**
* @dev Solv ABI for their router v1.
*/
interface ISolvBTCRouterV1 {
/**
* Subscribe with payment currency (i.e. WBTC) and receive SolvBTC.
* @param poolId SolvBTC fund ID.
* @param currencyAmount Amount of currency to be paid.
* @return shareValue Amount of SolvBTC to be received after subscription.
*/
function createSubscription(bytes32 poolId, uint256 currencyAmount) external returns (uint256 shareValue);
}

/**
* @dev Solv ABI for their router v2.
*/
Expand Down Expand Up @@ -109,3 +122,83 @@ contract XSolvBTCStrategy is IStrategyWithSlippageArgs, Context {
emit TokenOutput(address(xSolvBTC), xSolvBTCAmount);
}
}

contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context {
using SafeERC20 for IERC20;

ISolvBTCRouterV1 public immutable solvBTCRouterV1;
ISolvBTCRouterV2 public immutable solvBTCRouterV2;
bytes32 public immutable poolId;
IERC20 public immutable solvBTC;
IERC20 public immutable solvBTCJUP;

constructor(
ISolvBTCRouterV1 _solvBTCRouterV1,
ISolvBTCRouterV2 _solvBTCRouterV2,
bytes32 _poolId,
IERC20 _solvBTC,
IERC20 _solvBTCJUP
) {
solvBTCRouterV1 = _solvBTCRouterV1;
solvBTCRouterV2 = _solvBTCRouterV2;
poolId = _poolId;
solvBTC = _solvBTC;
solvBTCJUP = _solvBTCJUP;
}
Comment on lines +126 to +147
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add constructor invariants (non-zero addresses, non-zero poolId)

Prevent bricked deployments and ambiguous pool selection.

     constructor(
         ISolvBTCRouterV1 _solvBTCRouterV1,
         ISolvBTCRouterV2 _solvBTCRouterV2,
         bytes32 _poolId,
         IERC20 _solvBTC,
         IERC20 _solvBTCJUP
     ) {
+        require(address(_solvBTCRouterV1) != address(0) && address(_solvBTCRouterV2) != address(0), "router=0");
+        require(address(_solvBTC) != address(0) && address(_solvBTCJUP) != address(0), "token=0");
+        require(_poolId != bytes32(0), "poolId=0");
         solvBTCRouterV1 = _solvBTCRouterV1;
         solvBTCRouterV2 = _solvBTCRouterV2;
         poolId = _poolId;
         solvBTC = _solvBTC;
         solvBTCJUP = _solvBTCJUP;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context {
using SafeERC20 for IERC20;
ISolvBTCRouterV1 public immutable solvBTCRouterV1;
ISolvBTCRouterV2 public immutable solvBTCRouterV2;
bytes32 public immutable poolId;
IERC20 public immutable solvBTC;
IERC20 public immutable solvBTCJUP;
constructor(
ISolvBTCRouterV1 _solvBTCRouterV1,
ISolvBTCRouterV2 _solvBTCRouterV2,
bytes32 _poolId,
IERC20 _solvBTC,
IERC20 _solvBTCJUP
) {
solvBTCRouterV1 = _solvBTCRouterV1;
solvBTCRouterV2 = _solvBTCRouterV2;
poolId = _poolId;
solvBTC = _solvBTC;
solvBTCJUP = _solvBTCJUP;
}
contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context {
using SafeERC20 for IERC20;
ISolvBTCRouterV1 public immutable solvBTCRouterV1;
ISolvBTCRouterV2 public immutable solvBTCRouterV2;
bytes32 public immutable poolId;
IERC20 public immutable solvBTC;
IERC20 public immutable solvBTCJUP;
constructor(
ISolvBTCRouterV1 _solvBTCRouterV1,
ISolvBTCRouterV2 _solvBTCRouterV2,
bytes32 _poolId,
IERC20 _solvBTC,
IERC20 _solvBTCJUP
) {
require(address(_solvBTCRouterV1) != address(0) && address(_solvBTCRouterV2) != address(0), "router=0");
require(address(_solvBTC) != address(0) && address(_solvBTCJUP) != address(0), "token=0");
require(_poolId != bytes32(0), "poolId=0");
solvBTCRouterV1 = _solvBTCRouterV1;
solvBTCRouterV2 = _solvBTCRouterV2;
poolId = _poolId;
solvBTC = _solvBTC;
solvBTCJUP = _solvBTCJUP;
}
}
🤖 Prompt for AI Agents
In contracts/src/gateway/strategy/SolvStrategy.sol around lines 126 to 147, the
constructor lacks invariants allowing zero addresses or a zero poolId which can
brick deployments; add require checks asserting each router and token address is
not address(0) and poolId is not bytes32(0) with clear revert messages (e.g.,
"SolvStrategy: zero routerV1", "SolvStrategy: zero routerV2", "SolvStrategy:
zero poolId", "SolvStrategy: zero solvBTC", "SolvStrategy: zero solvBTCJUP")
before assigning state variables so invalid inputs are rejected immediately.


function handleGatewayMessageWithSlippageArgs(
IERC20 tokenSent,
uint256 amountIn,
address recipient,
StrategySlippageArgs memory args
) public override {
tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
tokenSent.safeIncreaseAllowance(address(solvBTCRouterV2), amountIn);

// Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically
// deposit input token into SolvBTC
uint256 solvBTCAmount = solvBTCRouterV2.deposit(
address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1)
);
Comment on lines +160 to +162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a deadline of block.timestamp + 1 is very tight. This pattern is used in both SolvBTCJUPStrategy and SolvBTCPlusStrategy. While the comment mentions the deposit is atomic, if the transaction is not mined in the next second (e.g., due to network congestion), it will revert. It's safer to use a slightly longer deadline (e.g., a few minutes) to make the strategies more robust.

        uint256 solvBTCAmount = solvBTCRouterV2.deposit(
            address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 180) // 3 minutes deadline
        );


solvBTC.safeIncreaseAllowance(address(solvBTCRouterV1), solvBTCAmount);

// now deposit SolvBTC for SolvBTC.JUP
uint256 solvBTCJUPAmount = solvBTCRouterV1.createSubscription(poolId, solvBTCAmount);

solvBTCJUP.safeTransfer(recipient, solvBTCJUPAmount);

emit TokenOutput(address(solvBTCJUP), solvBTCJUPAmount);
}
}

contract SolvBTCPlusStrategy is IStrategyWithSlippageArgs, Context {
using SafeERC20 for IERC20;

ISolvBTCRouterV2 public immutable solvBTCRouter;
IERC20 public immutable solvBTCPlus;

constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) {
solvBTCRouter = _solvBTCRouter;
solvBTCPlus = _solvBTCPlus;
}
Comment on lines +181 to +184
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Constructor invariants for Plus strategy

Match the safeguards added to JUP strategy.

     constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) {
+        require(address(_solvBTCRouter) != address(0), "router=0");
+        require(address(_solvBTCPlus) != address(0), "token=0");
         solvBTCRouter = _solvBTCRouter;
         solvBTCPlus = _solvBTCPlus;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) {
solvBTCRouter = _solvBTCRouter;
solvBTCPlus = _solvBTCPlus;
}
constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) {
require(address(_solvBTCRouter) != address(0), "router=0");
require(address(_solvBTCPlus) != address(0), "token=0");
solvBTCRouter = _solvBTCRouter;
solvBTCPlus = _solvBTCPlus;
}
🤖 Prompt for AI Agents
In contracts/src/gateway/strategy/SolvStrategy.sol around lines 181 to 184, the
constructor currently assigns solvBTCRouter and solvBTCPlus without any input
validation; add the same constructor invariants used in the JUP strategy by
requiring that the provided _solvBTCRouter and _solvBTCPlus addresses are
non-zero (e.g. require(address(_solvBTCRouter) != address(0) &&
address(_solvBTCPlus) != address(0), "Invalid constructor args")), then set the
state variables; optionally mark these variables immutable if they are not
modified elsewhere.


function handleGatewayMessageWithSlippageArgs(
IERC20 tokenSent,
uint256 amountIn,
address recipient,
StrategySlippageArgs memory args
) public override {
tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
tokenSent.safeIncreaseAllowance(address(solvBTCRouter), amountIn);

// Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically
uint256 solvBTCPlusAmount = solvBTCRouter.deposit(
address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1)
);

solvBTCPlus.safeTransfer(recipient, solvBTCPlusAmount);

emit TokenOutput(address(solvBTCPlus), solvBTCPlusAmount);
}
}
Comment on lines +126 to +204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new strategies SolvBTCJUPStrategy and SolvBTCPlusStrategy handle token transfers and interactions with external contracts. There's a risk that some amount of tokens (e.g., dust from swaps or tokens sent to the contract by mistake) could get stuck. It's a good practice to add a sweep function to both contracts that allows an owner/admin to withdraw any arbitrary ERC20 token to prevent loss of funds. This would involve adding an owner, an onlyOwner modifier, and the sweep function itself.

Comment on lines +175 to +204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Mirror hardening in SolvBTCPlusStrategy (approvals, deadline, recipient)

Same concerns as JUP strategy: exact approvals, clear after, longer deadline, non-zero recipient.

     function handleGatewayMessageWithSlippageArgs(
         IERC20 tokenSent,
         uint256 amountIn,
         address recipient,
         StrategySlippageArgs memory args
     ) public override {
-        tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
-        tokenSent.safeIncreaseAllowance(address(solvBTCRouter), amountIn);
+        require(recipient != address(0), "recipient=0");
+        tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
+        tokenSent.forceApprove(address(solvBTCRouter), amountIn);

-        // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically
         uint256 solvBTCPlusAmount = solvBTCRouter.deposit(
-            address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1)
+            address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 180)
         );
+        tokenSent.forceApprove(address(solvBTCRouter), 0);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
contracts/src/gateway/strategy/SolvStrategy.sol lines 175-204: the function uses
an open-ended allowance, a very short deadline, and doesn't validate recipient;
change to set allowance to zero then set exact approval for amountIn (use
safeApprove(0) then safeApprove(amountIn) or safeIncreaseAllowance after
zeroing), perform a require(recipient != address(0)) at the start, extend the
deadline to a reasonable buffer (e.g. uint64(block.timestamp + 300)) when
calling solvBTCRouter.deposit, and clear the allowance after the deposit
(safeApprove(address(solvBTCRouter), 0)) to avoid lingering approvals.

Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import {StrategySlippageArgs} from "../../../src/gateway/IStrategy.sol";
import {Constants} from "./Constants.sol";
import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";
import {IBedrockVault, BedrockStrategy} from "../../../src/gateway/strategy/BedrockStrategy.sol";
import {ForkedStrategyTemplateWbtc} from "./ForkedTemplate.sol";
import {ForkedStrategyTemplateWbtcOft} from "./ForkedTemplate.sol";

// Command to run this contract tests with Foundry:
// BOB_PROD_PUBLIC_RPC_URL=https://rpc.gobob.xyz/ forge test --match-contract BedrockStrategyForked -vv
contract BedrockStrategyForked is ForkedStrategyTemplateWbtc {
contract BedrockStrategyForked is ForkedStrategyTemplateWbtcOft {
function setUp() public {
super.simulateForkAndTransfer(
19911846, address(0x508A838922a93096C1Eb23FE21D8938BBd653Db6), Constants.DUMMY_SENDER, 1e8
21930500, address(0xa79a356B01ef805B3089b4FE67447b96c7e6DD4C), Constants.DUMMY_SENDER, 1e8
);
}

Expand Down
1 change: 1 addition & 0 deletions contracts/test/gateway/e2e-strategy-tests/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pragma solidity ^0.8.13;
library Constants {
// Used for providing a uniform interface for functions that deal with both ERC20 and native tokens.
address constant WBTC_ADDRESS = 0x03C7054BCB39f7b2e5B2c7AcB37583e32D70Cfa3;
address constant WBTC_OFT_ADDRESS = 0x0555E30da8f98308EdB960aa94C0Db47230d2B9c;
address constant TBTC_ADDRESS = 0xBBa2eF945D523C4e2608C9E1214C2Cc64D4fc2e2;
address constant DUMMY_SENDER = 0x999999cf1046e68e36E1aA2E0E07105eDDD1f08E;
address constant DUMMY_RECEIVER = 0x0000000000000000000000000000000000000001;
Expand Down
14 changes: 14 additions & 0 deletions contracts/test/gateway/e2e-strategy-tests/ForkedTemplate.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,20 @@ abstract contract ForkedStrategyTemplateWbtc is Test {
}
}

abstract contract ForkedStrategyTemplateWbtcOft is Test {
IERC20 public token;

constructor() {
token = IERC20(Constants.WBTC_OFT_ADDRESS);
}

function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public {
vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock);
vm.prank(sender);
token.transfer(receiver, amount);
}
}
Comment on lines +27 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This new abstract contract ForkedStrategyTemplateWbtcOft is very similar to ForkedStrategyTemplateWbtc and ForkedStrategyTemplateTbtc. To avoid code duplication and improve maintainability, consider refactoring them into a single generic base template that accepts the token address as a constructor argument.

For example:

abstract contract ForkedStrategyTemplate is Test {
    IERC20 public token;

    constructor(address tokenAddress) {
        token = IERC20(tokenAddress);
    }

    function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public {
        vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock);
        vm.prank(sender);
        token.transfer(receiver, amount);
    }
}


abstract contract ForkedStrategyTemplateTbtc is Test {
IERC20 public token;

Expand Down
47 changes: 43 additions & 4 deletions contracts/test/gateway/e2e-strategy-tests/SolvStrategyForked.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,24 @@ import {stdStorage, StdStorage, Test, console} from "forge-std/Test.sol";
using stdStorage for StdStorage;

import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";
import {ISolvBTCRouterV2, SolvBTCStrategy, XSolvBTCStrategy} from "../../../src/gateway/strategy/SolvStrategy.sol";
import {
ISolvBTCRouterV1,
ISolvBTCRouterV2,
SolvBTCStrategy,
XSolvBTCStrategy,
SolvBTCJUPStrategy,
SolvBTCPlusStrategy
} from "../../../src/gateway/strategy/SolvStrategy.sol";
import {StrategySlippageArgs} from "../../../src/gateway/IStrategy.sol";
import {Constants} from "./Constants.sol";
import {ForkedStrategyTemplateWbtc} from "./ForkedTemplate.sol";
import {ForkedStrategyTemplateWbtcOft} from "./ForkedTemplate.sol";

// Command to run this contract tests with Foundry:
// BOB_PROD_PUBLIC_RPC_URL=https://rpc.gobob.xyz/ forge test --match-contract SolvStrategyForked -vv
contract SolvStrategyForked is ForkedStrategyTemplateWbtc {
contract SolvStrategyForked is ForkedStrategyTemplateWbtcOft {
function setUp() public {
super.simulateForkAndTransfer(
19911846, address(0x508A838922a93096C1Eb23FE21D8938BBd653Db6), Constants.DUMMY_SENDER, 1e8
21939620, address(0xa79a356B01ef805B3089b4FE67447b96c7e6DD4C), Constants.DUMMY_SENDER, 1e8
);
}

Expand Down Expand Up @@ -47,4 +54,36 @@ contract SolvStrategyForked is ForkedStrategyTemplateWbtc {

assertApproxEqAbs(xSolvBTC.balanceOf(Constants.DUMMY_RECEIVER), 1e18, 1e18 / 100);
}

function testSolvBTCJUPStrategy() public {
IERC20 solvBTC = IERC20(0x541FD749419CA806a8bc7da8ac23D346f2dF8B77);
IERC20 solvBTCJUP = IERC20(0x6b062AA7F5FC52b530Cb13967aE2E6bc0D8Dd3E4);
bytes32 poolId = 0x6f113a39a50769de40d4f2e7e46b6a4c6d7774e2c3943ced2dbcb25e626d1d04;
ISolvBTCRouterV1 solvBTCRouterV1 = ISolvBTCRouterV1(0xbA46FcC16B464D9787314167bDD9f1Ce28405bA1);
ISolvBTCRouterV2 solvBTCRouterV2 = ISolvBTCRouterV2(0x56a4d805d7A292f03Ead5Be31E0fFB8f7d0E3B48);
SolvBTCJUPStrategy strategy =
new SolvBTCJUPStrategy(solvBTCRouterV1, solvBTCRouterV2, poolId, solvBTC, solvBTCJUP);

vm.startPrank(Constants.DUMMY_SENDER);
token.approve(address(strategy), 1e8);

strategy.handleGatewayMessageWithSlippageArgs(token, 1e8, Constants.DUMMY_RECEIVER, StrategySlippageArgs(0));
vm.stopPrank();

assertApproxEqAbs(solvBTCJUP.balanceOf(Constants.DUMMY_RECEIVER), 1e18, 1e18 / 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The 10% delta (1e18 / 10) used in this assertion is quite large. This might be expected due to fees or slippage in the two-step swap process, but it could also hide potential issues. It would be beneficial to add a comment explaining why such a large deviation is expected. If it's not expected, this might need further investigation.

}

function testSolvBTCPlusStrategy() public {
IERC20 solvBTCPlus = IERC20(0x4Ca70811E831db42072CBa1f0d03496EF126fAad);
SolvBTCPlusStrategy strategy =
new SolvBTCPlusStrategy(ISolvBTCRouterV2(0x56a4d805d7A292f03Ead5Be31E0fFB8f7d0E3B48), solvBTCPlus);

vm.startPrank(Constants.DUMMY_SENDER);
token.approve(address(strategy), 1e8);

strategy.handleGatewayMessageWithSlippageArgs(token, 1e8, Constants.DUMMY_RECEIVER, StrategySlippageArgs(0));
vm.stopPrank();

assertApproxEqAbs(solvBTCPlus.balanceOf(Constants.DUMMY_RECEIVER), 1e18, 1e18 / 100);
}
}
Loading