-
Notifications
You must be signed in to change notification settings - Fork 1
feat: BungeeApproveAndBridge #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8dc4089
47fce30
d843e7f
bff4da2
99e50d6
f83bdd6
7ab7ddf
87ba711
f880024
40a8dd8
92e4d1e
d0b4b74
c3eb149
00f83ab
feffeab
fb92970
fb5f94a
9de1221
f35c950
123bb92
ead2d04
e59ca9c
89a6929
0422679
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "solidity.remappings": [ | ||
| "ds-test/=lib/forge-std/lib/ds-test/src/", | ||
| "forge-std/=lib/forge-std/src", | ||
| "src/=./src/", | ||
| "permit2/=lib/permit2/", | ||
| "solmate/=lib/solmate/" | ||
| ] | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| pragma solidity ^0.8; | ||
|
|
||
| import {Script, console} from "forge-std/Script.sol"; | ||
| import {BungeeApproveAndBridge} from "src/BungeeApproveAndBridge.sol"; | ||
|
|
||
| contract DeployScript is Script { | ||
| BungeeApproveAndBridge public bungeeApproveAndBridge; | ||
|
|
||
| address constant SOCKET_GATEWAY = 0x3a23F943181408EAC424116Af7b7790c94Cb97a5; | ||
|
|
||
| function run() public { | ||
| address txOrigin = msg.sender; | ||
| address create2Deployer = 0x4e59b44847b379578588920cA78FbF26c0B4956C; // foundry uses this contract by default | ||
| bytes32 salt = keccak256(abi.encode(uint256(9999999999999999999999))); // change this before deploying if needed | ||
| console.log("Deployer: ", txOrigin); | ||
| console.log("Create2 Deployer: ", create2Deployer); | ||
| console.logBytes32(salt); | ||
|
|
||
| address computedAddress = vm.computeCreate2Address( | ||
| salt, | ||
| keccak256(abi.encodePacked(type(BungeeApproveAndBridge).creationCode, abi.encode(SOCKET_GATEWAY))), | ||
| create2Deployer | ||
| ); | ||
| console.log("Computed address: ", computedAddress); | ||
|
|
||
| bungeeApproveAndBridge = BungeeApproveAndBridge(deploy(salt)); | ||
| console.log("Deployed BungeeApproveAndBridge at: ", address(bungeeApproveAndBridge)); | ||
| } | ||
|
|
||
| function deploy(bytes32 salt) public returns (address addr) { | ||
| vm.broadcast(); | ||
| BungeeApproveAndBridge _bungeeApproveAndBridge = new BungeeApproveAndBridge{salt: salt}(SOCKET_GATEWAY); | ||
| addr = address(_bungeeApproveAndBridge); | ||
| console.log("Deployed contract to: %s", address(addr)); | ||
|
|
||
| return addr; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| pragma solidity ^0.8; | ||
|
|
||
| import {ApproveAndBridge, IERC20} from "./mixin/ApproveAndBridge.sol"; | ||
| import {Math} from "./vendored/Math.sol"; | ||
|
|
||
| /// ! @dev UNAUDITED UNTESTED Do not use in production | ||
| /// @dev Performs two steps before bridging via SocketGateway: | ||
| /// 1. Modify input amount in calldata | ||
| /// 2. Modify output amount in calldata | ||
| /// 3. Call SocketGateway.fallback() with the modified calldata | ||
| contract BungeeApproveAndBridge is ApproveAndBridge { | ||
| error InvalidInput(); | ||
| error PositionOutOfBounds(); | ||
| error BridgeFailed(); | ||
|
|
||
| /// @dev ModifyCalldataParams is a struct that contains information required to modify SocketGateway calldata | ||
| /// @dev the input amount index, modify output flag, and output amount index | ||
| struct ModifyCalldataParams { | ||
| uint256 inputAmountIdx; | ||
| bool modifyOutput; | ||
| uint256 outputAmountIdx; | ||
| } | ||
|
|
||
| /// @dev routeIds on SocketGateway are 4 bytes | ||
| uint8 private constant ROUTE_ID_BYTES_LENGTH = 4; | ||
| /// @dev there are 3 params in ModifyCalldataParams | ||
| uint8 private constant MODIFY_CALLDATA_PARAMS_COUNT = 3; | ||
| /// @dev each ModifyCalldataParams is 32 bytes | ||
| uint8 private constant MODIFY_CALLDATA_LENGTH_BYTES = 32; | ||
| /// @dev total length of the modify calldata bytes | ||
| uint8 private constant MODIFY_CALLDATA_LENGTH = MODIFY_CALLDATA_PARAMS_COUNT * MODIFY_CALLDATA_LENGTH_BYTES; | ||
| /// @dev minimum length of the data payload | ||
| /// @dev should atleast include the routeId and the ModifyCalldataParams | ||
| uint8 private constant MIN_DATA_LENGTH = ROUTE_ID_BYTES_LENGTH + MODIFY_CALLDATA_LENGTH; | ||
|
|
||
| /// @dev SocketGateway address | ||
| address public immutable SOCKET_GATEWAY; | ||
|
|
||
| constructor(address socketGateway_) { | ||
| require(socketGateway_.code.length > 0, "Socket gateway contract not deployed"); | ||
|
|
||
| SOCKET_GATEWAY = socketGateway_; | ||
| } | ||
|
|
||
| /** | ||
| * @notice Approval should be given to the SocketGateway address | ||
| * @dev Returns the SocketGateway address | ||
| */ | ||
| function bridgeApprovalTarget() public view override returns (address) { | ||
| return address(SOCKET_GATEWAY); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Bridge the token via SocketGateway | ||
| * @dev Modifies SocketGateway calldata to modify the input and output amounts before bridging | ||
| * @param token The token to bridge | ||
| * @param amount The amount of token to bridge | ||
| * @param nativeTokenExtraFee extra fee in native token, if any | ||
| * @param data encoded bytes including SocketGateway calldata and ModifyCalldataParams | ||
| */ | ||
| function bridge(IERC20 token, uint256 amount, uint256 nativeTokenExtraFee, bytes calldata data) internal override { | ||
| // decode & parse data to find positions in calldata to modify | ||
| bytes memory modifiedCalldata = _parseAndModifyCalldata(amount, data); | ||
|
|
||
| // execute using the modified calldata via SocketGateway.fallback() | ||
| (bool success,) = address(token) == NATIVE_TOKEN_ADDRESS | ||
| ? address(SOCKET_GATEWAY).call{value: amount + nativeTokenExtraFee}(modifiedCalldata) | ||
| : address(SOCKET_GATEWAY).call{value: nativeTokenExtraFee}(modifiedCalldata); | ||
| if (!success) revert BridgeFailed(); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Parses and modifies the calldata to modify the input and output amounts before bridging | ||
| * @param amount Updated input amount to use to modify the calldata | ||
| * @param data encoded bytes including SocketGateway calldata and ModifyCalldataParams | ||
| * @return modifiedCalldata The modified calldata | ||
| */ | ||
| function _parseAndModifyCalldata(uint256 amount, bytes calldata data) internal pure returns (bytes memory) { | ||
| // Parse the data into route calldata and ModifyCalldataParams | ||
| (bytes memory routeCalldata, ModifyCalldataParams memory modifyCalldataParams) = _parseCalldata(data); | ||
|
|
||
| // Read the original input amount from the calldata | ||
| // before modifying input amount | ||
| uint256 originalInput = _readUint256({_data: routeCalldata, _index: modifyCalldataParams.inputAmountIdx}); | ||
|
|
||
| // Replace the input amount in the calldata | ||
| bytes memory modifiedCalldata = | ||
| _replaceUint256({_original: routeCalldata, _start: modifyCalldataParams.inputAmountIdx, _amount: amount}); | ||
|
|
||
| // Optionally replace the output amount if required | ||
| // in case of bridges like Across, need to modify both input and output amounts | ||
| // - decode current input and output amounts from calldata | ||
| // - calculate and apply the percentage diff bw new and old input amount on the old output amount | ||
| // - replace the output amount at the index with the new amount | ||
| // - assumes output amount is always uint256 in SocketGateway impls | ||
| if (modifyCalldataParams.modifyOutput) { | ||
fedgiac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| uint256 originalOutput = _readUint256({_data: routeCalldata, _index: modifyCalldataParams.outputAmountIdx}); | ||
| uint256 newOutput = _applyPctDiff({_base: originalInput, _compare: amount, _target: originalOutput}); | ||
anxolin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| modifiedCalldata = _replaceUint256({ | ||
| _original: modifiedCalldata, | ||
| _start: modifyCalldataParams.outputAmountIdx, | ||
| _amount: newOutput | ||
| }); | ||
| } | ||
|
|
||
| return modifiedCalldata; | ||
| } | ||
fedgiac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * @dev Parses the calldata to extract the route calldata and ModifyCalldataParams | ||
| * @param _data The calldata to parse | ||
| * @return routeCalldata The SocketGateway route calldata | ||
| * @return modifyCalldataParams The ModifyCalldataParams | ||
| */ | ||
| function _parseCalldata(bytes calldata _data) internal pure returns (bytes memory, ModifyCalldataParams memory) { | ||
| // calldata should have minimum of routeId and ModifyCalldataParams | ||
| if (_data.length < MIN_DATA_LENGTH) revert InvalidInput(); | ||
| uint256 routeCalldataLength = _data.length - MODIFY_CALLDATA_LENGTH; | ||
|
|
||
| // Extract the route execution calldata | ||
| bytes memory routeCalldata = _data[:routeCalldataLength]; | ||
|
|
||
| // Extract the ModifyCalldataParams | ||
| ModifyCalldataParams memory modifyCalldataParams; | ||
| (modifyCalldataParams.inputAmountIdx, modifyCalldataParams.modifyOutput, modifyCalldataParams.outputAmountIdx) = | ||
| abi.decode(_data[routeCalldataLength:], (uint256, bool, uint256)); | ||
|
|
||
| return (routeCalldata, modifyCalldataParams); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Replaces a uint256 at a given position in a bytes data with a new uint256 | ||
| * @dev Directly modifies the original bytes data in-place without creating a new copy | ||
| */ | ||
| function _replaceUint256(bytes memory _original, uint256 _start, uint256 _amount) | ||
| internal | ||
| pure | ||
| returns (bytes memory) | ||
| { | ||
| // check if the _start is out of bounds | ||
| if (_start + 32 > _original.length) revert PositionOutOfBounds(); | ||
|
|
||
| // Directly modify externalData in-place without creating a new copy | ||
| assembly { | ||
| // Calculate position in memory where we need to write the new amount | ||
| // Write the amount at that position | ||
| mstore(add(add(_original, 32), _start), _amount) | ||
| } | ||
|
|
||
| return _original; | ||
| } | ||
|
Comment on lines
+136
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAssembly operations look correct but need comprehensive testing. The Consider adding unit tests specifically for this function to verify:
🏁 Script executed: #!/bin/bash
# Search for existing tests covering _replaceUint256 function
rg -A 10 -B 5 "_replaceUint256" --type sol
rg -n "_replaceUint256.*test" .Length of output: 104 🏁 Script executed: #!/bin/bash
# Search for occurrences of _replaceUint256 across all Solidity files
rg -n "_replaceUint256" -g "*.sol"
# Search for tests referencing the function in JS/TS test files
rg -n "_replaceUint256" -g "test/**/*.js" -g "test/**/*.ts"Length of output: 600 Add comprehensive unit tests for I didn’t find any existing tests covering this function in • Verify correct replacement of a 32-byte word at various byte offsets (start, middle, end) File to cover:
🤖 Prompt for AI Agents
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@sebastiantf Perfect! I can see you've added comprehensive tests for ✅ Bounds checking: Tests verify The test coverage looks thorough and addresses all the memory safety concerns I raised. Thanks for implementing these comprehensive tests! ( ..) 🏁 Script executed: Length of output: 70979 @sebastiantf Perfect! I can see you've added comprehensive tests for ✅ Bounds checking: Tests verify The test coverage looks thorough and addresses all the memory safety concerns I raised. Thanks for implementing these comprehensive tests! ( ..) If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
||
|
|
||
| /** | ||
| * @dev Reads a uint256 at a given byte index in a bytes array | ||
| */ | ||
| function _readUint256(bytes memory _data, uint256 _index) internal pure returns (uint256 value) { | ||
| if (_data.length < _index + 32) revert PositionOutOfBounds(); | ||
| assembly { | ||
| value := mload(add(add(_data, 0x20), _index)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @dev Applies a percentage difference to a target number | ||
| */ | ||
| function _applyPctDiff(uint256 _base, uint256 _compare, uint256 _target) internal pure returns (uint256) { | ||
| if (_base == 0) revert InvalidInput(); | ||
| return Math.mulDiv({x: _target, y: _compare, denominator: _base}); | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| pragma solidity ^0.8; | ||
|
|
||
| import {IERC20} from "../vendored/IERC20.sol"; | ||
|
|
||
| interface IApproveAndBridge { | ||
| function approveAndBridge(IERC20 token, uint256 minAmount, uint256 nativeTokenExtraFee, bytes calldata data) | ||
| external; | ||
|
|
||
| function bridgeApprovalTarget() external view returns (address); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| pragma solidity ^0.8; | ||
|
|
||
| import {IApproveAndBridge} from "../interface/IApproveAndBridge.sol"; | ||
| import {IERC20} from "../vendored/IERC20.sol"; | ||
| import {SafeERC20} from "../vendored/SafeERC20.sol"; | ||
|
|
||
| abstract contract ApproveAndBridge is IApproveAndBridge { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| error MinAmountNotMet(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice to make a test for premature hook execution
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have added some more unit tests here: 89a6929 |
||
|
|
||
| /// @dev Address used to represent the native token | ||
| address public constant NATIVE_TOKEN_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; | ||
|
|
||
| /** | ||
| * @dev This function isn't intended to be called directly, it should be delegatecalled instead. | ||
| * @param token The token to bridge | ||
| * @param minAmount The minimum amount of tokens to bridge. minAmount should not be too small if the sell amount is big | ||
| * @param nativeTokenExtraFee The extra fee to pay in native tokens | ||
| * @param data The data to pass to the bridge | ||
| */ | ||
| function approveAndBridge(IERC20 token, uint256 minAmount, uint256 nativeTokenExtraFee, bytes calldata data) | ||
| external | ||
| { | ||
| // get the balance of the token | ||
| uint256 balance = address(token) == NATIVE_TOKEN_ADDRESS | ||
| // if native token, reduce the extra fee from balance | ||
| // if not enough balance, it will underflow and revert | ||
| ? address(this).balance - nativeTokenExtraFee | ||
| : token.balanceOf(address(this)); | ||
|
|
||
| // check if the balance is greater than the minAmount | ||
| if (balance < minAmount) revert MinAmountNotMet(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think is an issue, but probably we should add in the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comment: 0422679 |
||
|
|
||
| // approve the bridgeApprovalTarget if ERC20 | ||
| if (address(token) != NATIVE_TOKEN_ADDRESS) { | ||
| token.forceApprove(bridgeApprovalTarget(), balance); | ||
| } | ||
|
|
||
| // bridge the token | ||
| bridge(token, balance, nativeTokenExtraFee, data); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Returns the address of the contract that should be approved to bridge the token | ||
| */ | ||
| function bridgeApprovalTarget() public view virtual returns (address); | ||
|
|
||
| /** | ||
| * @dev Bridges the token | ||
| */ | ||
| function bridge(IERC20 token, uint256 amount, uint256 nativeTokenExtraFee, bytes calldata data) internal virtual; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.