feat: standard bridge contract improvements (#83)

* remove whitelist precompiles * Update DeployStandardBridge.s.sol * Update DeployStandardBridge.s.sol * make transferIdx indexed and adjust tests * add transferFinalizedIdx and relevant tests * update contract addrs * Update DeployScripts.s.sol * Update SettlementGateway.sol * Revert "Update SettlementGateway.sol" This reverts commit bfa9b4f. * rm outdated todo
primev · Feb 20, 2024 · 6adab5a · 6adab5a
1 parent 560b093
commit 6adab5a
Show file tree

Hide file tree

Showing 7 changed files with 305 additions and 100 deletions.
diff --git a/contracts/Whitelist.sol b/contracts/Whitelist.sol
@@ -4,16 +4,13 @@ pragma solidity ^0.8.15;
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
 // Contract that allows an admin to add/remove addresses from the whitelist,
-// and allows whitelisted addresses to mint/burn native tokens.
+// and allows whitelisted addresses to mint native tokens.
+//
+// The whitelist contract's create2 address must be funded on genesis.
 contract Whitelist is Ownable {
 
     mapping(address => bool) public whitelistedAddresses;
 
-    // Mint/burn precompile addresses.
-    // See: https://github.com/primevprotocol/go-ethereum/blob/03ae168c6ac15dda8c5a3f123e2b9f3350aad613/core/vm/contracts.go
-    address constant MINT = address(0x89);
-    address constant BURN = address(0x90);
-
     constructor(address _owner) Ownable() {
         _transferOwnership(_owner);
     }
@@ -30,29 +27,13 @@ contract Whitelist is Ownable {
         return whitelistedAddresses[_address];
     }
 
-    // Mints native tokens if the sender is whitelisted.
-    // See: https://github.com/primevprotocol/go-ethereum/blob/precompile-updates/core/vm/contracts_with_ctx.go#L83
+    // "Mints" native tokens (transfer ether from this contract) if the sender is whitelisted.
     function mint(address _mintTo, uint256 _amount) external {
         require(isWhitelisted(msg.sender), "Sender is not whitelisted");
-        bool success;
-        (success, ) = MINT.call{value: 0, gas: gasleft()}(
-            abi.encode(_mintTo, _amount)
-        );
-        require(success, "Native mint failed");
+        require(address(this).balance >= _amount, "Insufficient contract balance");
+        payable(_mintTo).transfer(_amount);
     }
 
-    // Burns native tokens if the sender is whitelisted.
-    function burn(address _burnFrom, uint256 _amount) external {
-        require(isWhitelisted(msg.sender), "Sender is not whitelisted");
-
-        // require _burnFrom has enough balance. This check is NOT done at the precompile level.
-        // Reason: https://github.com/primevprotocol/go-ethereum/blob/8735a9bbe6965ed68371472cb0794d8659a94428/core/vm/contracts_with_ctx.go#L115
-        require(_burnFrom.balance >= _amount, "Insufficient balance");
-
-        bool success;
-        (success, ) = BURN.call{value: 0, gas: gasleft()}(
-            abi.encode(_burnFrom, _amount) 
-        );
-        require(success, "Native burn failed");
-    }
+    // Receiver for native tokens to be "burnt"
+    receive() external payable {}
 }
diff --git a/contracts/standard-bridge/Gateway.sol b/contracts/standard-bridge/Gateway.sol
@@ -8,9 +8,13 @@ import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
  */
 abstract contract Gateway is Ownable {   
 
-    // @dev index for tracking transfers.
+    // @dev index for tracking transfer initiations.
     // Also total number of transfers initiated from this gateway.
-    uint256 public transferIdx;
+    uint256 public transferInitiatedIdx;
+
+    // @dev index for tracking transfer finalizations.
+    // Also total number of transfers finalized on this gateway.
+    uint256 public transferFinalizedIdx;
 
     // @dev Address of relayer account. 
     address public immutable relayer;
@@ -27,16 +31,18 @@ abstract contract Gateway is Ownable {
         relayer = _relayer;
         finalizationFee = _finalizationFee;
         counterpartyFee = _counterpartyFee;
+        transferInitiatedIdx = 0;
+        transferFinalizedIdx = 1; // First expected transfer index is 1
         _transferOwnership(_owner);
     }
 
     function initiateTransfer(address _recipient, uint256 _amount
     ) external payable returns (uint256 returnIdx) {
         require(_amount >= counterpartyFee, "Amount must cover counterpartys finalization fee");
-        ++transferIdx;
         _decrementMsgSender(_amount);
-        emit TransferInitiated(msg.sender, _recipient, _amount, transferIdx);
-        return transferIdx;
+        ++transferInitiatedIdx;
+        emit TransferInitiated(msg.sender, _recipient, _amount, transferInitiatedIdx);
+        return transferInitiatedIdx;
     }
     // @dev where _decrementMsgSender is implemented by inheriting contract.
     function _decrementMsgSender(uint256 _amount) internal virtual;
@@ -49,9 +55,11 @@ abstract contract Gateway is Ownable {
     function finalizeTransfer(address _recipient, uint256 _amount, uint256 _counterpartyIdx
     ) external onlyRelayer {
         require(_amount >= finalizationFee, "Amount must cover finalization fee");
+        require(_counterpartyIdx == transferFinalizedIdx, "Invalid counterparty index. Transfers must be relayed FIFO");
         uint256 amountAfterFee = _amount - finalizationFee;
         _fund(amountAfterFee, _recipient);
         _fund(finalizationFee, relayer);
+        ++transferFinalizedIdx;
         emit TransferFinalized(_recipient, _amount, _counterpartyIdx);
     }
     // @dev where _fund is implemented by inheriting contract.
@@ -65,7 +73,7 @@ abstract contract Gateway is Ownable {
      * @param transferIdx Current index of this gateway.
      */
     event TransferInitiated(
-        address indexed sender, address indexed recipient, uint256 amount, uint256 transferIdx);
+        address indexed sender, address indexed recipient, uint256 amount, uint256 indexed transferIdx);
 
     /**
      * @dev Emitted when a transfer is finalized.
@@ -74,5 +82,5 @@ abstract contract Gateway is Ownable {
      * @param counterpartyIdx Index of counterpary gateway when transfer was initiated.
      */
     event TransferFinalized(
-        address indexed recipient, uint256 amount, uint256 counterpartyIdx);
+        address indexed recipient, uint256 amount, uint256 indexed counterpartyIdx);
 }
diff --git a/contracts/standard-bridge/SettlementGateway.sol b/contracts/standard-bridge/SettlementGateway.sol
@@ -7,7 +7,7 @@ import {IWhitelist} from "../interfaces/IWhitelist.sol";
 contract SettlementGateway is Gateway{
 
     // Assuming deployer is 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266,
-    // whitelist's create2 addr should be 0x5D1415C0973034d162F5FEcF19B50dA057057e29.
+    // whitelist's create2 addr should be 0x57508f0B0f3426758F1f3D63ad4935a7c9383620.
     // This variable is not hardcoded for testing purposes.
     address public immutable whitelistAddr;
 
@@ -16,13 +16,15 @@ contract SettlementGateway is Gateway{
             whitelistAddr = _whitelistAddr;
         }
 
-    // Burns native ether on settlement chain, 
+    // Burns native ether on settlement chain by sending it to the whitelist contract,
     // there should be equiv ether on L1 which will be UNLOCKED during finalization.
     function _decrementMsgSender(uint256 _amount) internal override {
-        IWhitelist(whitelistAddr).burn(msg.sender, _amount);
+        require(msg.value == _amount, "Incorrect Ether value sent");
+        (bool success, ) = whitelistAddr.call{value: msg.value}("");
+        require(success, "Failed to send Ether");
     }
 
-    // Mints native ether on settlement chain, 
+    // Mints native ether on settlement chain via whitelist contract,
     // there should be equiv ether on L1 which remains LOCKED.
     function _fund(uint256 _amount, address _toFund) internal override {
         IWhitelist(whitelistAddr).mint(_toFund, _amount);

diff --git a/scripts/DeployScripts.s.sol b/scripts/DeployScripts.s.sol
@@ -77,7 +77,7 @@ contract DeployScript is Script, Create2Deployer {
 contract DeployWhitelist is Script, Create2Deployer {
     function run() external {
 
-        address expectedWhiteListAddr = 0x5D1415C0973034d162F5FEcF19B50dA057057e29;
+        address expectedWhiteListAddr = 0x57508f0B0f3426758F1f3D63ad4935a7c9383620;
         if (isContractDeployed(expectedWhiteListAddr)) {
             console.log("Whitelist already deployed to:", expectedWhiteListAddr);
             return;
@@ -89,7 +89,7 @@ contract DeployWhitelist is Script, Create2Deployer {
         checkDeployer();
 
         address hypERC20Addr = vm.envAddress("HYP_ERC20_ADDR");
-        require(hypERC20Addr != address(0), "Whitelist address not provided");
+        require(hypERC20Addr != address(0), "Address to whitelist not provided");
 
         // Forge deploy with salt uses create2 proxy from https://github.com/primevprotocol/deterministic-deployment-proxy
         bytes32 salt = 0x8989000000000000000000000000000000000000000000000000000000000000;

diff --git a/scripts/DeployStandardBridge.s.sol b/scripts/DeployStandardBridge.s.sol
@@ -9,7 +9,7 @@ contract DeploySettlementGateway is Script, Create2Deployer {
     function run() external {
 
         // Note this addr is dependant on values given to contract constructor
-        address expectedAddr = 0x0D70A44c81a27f33a36C334bFEA8bBBD8A7d58AA;
+        address expectedAddr = 0xc1f93bE11D7472c9B9a4d87B41dD0a491F1fbc75;
         if (isContractDeployed(expectedAddr)) {
             console.log("Standard bridge gateway on settlement chain already deployed to:",
                 expectedAddr);
@@ -24,11 +24,11 @@ contract DeploySettlementGateway is Script, Create2Deployer {
         // Forge deploy with salt uses create2 proxy from https://github.com/primevprotocol/deterministic-deployment-proxy
         bytes32 salt = 0x8989000000000000000000000000000000000000000000000000000000000000;
 
-        address whitelistAddr = 0x5D1415C0973034d162F5FEcF19B50dA057057e29;
+        address expectedWhitelistAddr = 0x57508f0B0f3426758F1f3D63ad4935a7c9383620;
         address relayerAddr = vm.envAddress("RELAYER_ADDR");
 
         SettlementGateway gateway = new SettlementGateway{salt: salt}(
-            whitelistAddr,
+            expectedWhitelistAddr,
             msg.sender, // Owner
             relayerAddr,
             1, 1); // Fees set to 1 wei for now
@@ -43,7 +43,7 @@ contract DeployL1Gateway is Script, Create2Deployer {
     function run() external {
 
         // Note this addr is dependant on values given to contract constructor
-        address expectedAddr = 0x38b7e046bd971B4123974Bc78DcB0D7C680d85d2;
+        address expectedAddr = 0x1a18dfEc4f2B66207b1Ad30aB5c7A0d62Ef4A40b;
         if (isContractDeployed(expectedAddr)) {
             console.log("Standard bridge gateway on l1 already deployed to:",
                 expectedAddr);

diff --git a/test/standard-bridge/L1GatewayTest.sol b/test/standard-bridge/L1GatewayTest.sol
@@ -30,15 +30,16 @@ contract L1GatewayTest is Test {
 
     // Expected event signature emitted in initiateTransfer()
     event TransferInitiated(
-        address indexed sender, address indexed recipient, uint256 amount, uint256 transferIdx);
+        address indexed sender, address indexed recipient, uint256 amount, uint256 indexed transferIdx);
 
-    function test_InitiateTransfer() public {
+    function test_InitiateTransferSuccess() public {
         vm.deal(bridgeUser, 100 ether);
         uint256 amount = 7 ether;
 
         // Initial assertions
         assertEq(address(bridgeUser).balance, 100 ether);
-        assertEq(l1Gateway.transferIdx(), 0);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
 
         // Set up expectation for event
         vm.expectEmit(true, true, true, true);
@@ -50,24 +51,66 @@ contract L1GatewayTest is Test {
 
         // Assertions after call
         assertEq(address(bridgeUser).balance, 93 ether); 
-        assertEq(l1Gateway.transferIdx(), 1);
+        assertEq(l1Gateway.transferInitiatedIdx(), 1);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
         assertEq(returnedIdx, 1);
     }
 
-    function TestAmountTooSmallForCounterpartyFee() public {
+    function test_InitiateTransferAmountTooSmallForCounterpartyFee() public {
         vm.deal(bridgeUser, 100 ether);
         vm.deal(address(l1Gateway), 1 ether);
+
         assertEq(address(bridgeUser).balance, 100 ether);
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
         vm.expectRevert("Amount must cover counterpartys finalization fee");
         vm.prank(bridgeUser);
         l1Gateway.initiateTransfer{value: 0.04 ether}(bridgeUser, 0.04 ether);
+
+        assertEq(address(bridgeUser).balance, 100 ether);
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+    }
+
+    function test_InitiateTransferUserInsufficientBalance() public {
+        vm.deal(bridgeUser, 0.01 ether);
+
+        assertEq(address(bridgeUser).balance, 0.01 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
+        vm.expectRevert();
+        vm.prank(bridgeUser);
+        l1Gateway.initiateTransfer{value: 0.9 ether}(bridgeUser, 0.9 ether);
+
+        assertEq(address(bridgeUser).balance, 0.01 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+    }
+
+    function test_InitiateTransferValueMismatch() public {
+        vm.deal(bridgeUser, 100 ether);
+
+        assertEq(address(bridgeUser).balance, 100 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
+        vm.expectRevert("Incorrect Ether value sent");
+        vm.prank(bridgeUser);
+        l1Gateway.initiateTransfer{value: 0.8 ether}(bridgeUser, 0.9 ether);
+
+        assertEq(address(bridgeUser).balance, 100 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
     }
 
     event TransferFinalized(
-        address indexed recipient, uint256 amount, uint256 counterpartyIdx);
+        address indexed recipient, uint256 amount, uint256 indexed counterpartyIdx);
 
-    function test_FinalizeTransfer() public {
-        // These values are trusted from relayer
+    function test_FinalizeTransferSuccess() public {
         uint256 amount = 4 ether;
         uint256 counterpartyIdx = 1;
 
@@ -79,7 +122,8 @@ contract L1GatewayTest is Test {
         assertEq(address(l1Gateway).balance, 5 ether);
         assertEq(relayer.balance, 5 ether);
         assertEq(bridgeUser.balance, 0 ether);
-        assertEq(l1Gateway.transferIdx(), 0);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
 
         // Set up expectation for event
         vm.expectEmit(true, true, true, true);
@@ -93,23 +137,79 @@ contract L1GatewayTest is Test {
         assertEq(address(l1Gateway).balance, 1 ether);
         assertEq(relayer.balance, 5.1 ether);
         assertEq(bridgeUser.balance, 3.9 ether);
-        assertEq(l1Gateway.transferIdx(), 0);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 2);
     }
 
     function test_OnlyRelayerCanCallFinalizeTransfer() public {
         uint256 amount = 0.1 ether;
         vm.deal(address(l1Gateway), 1 ether);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
         vm.expectRevert("Only relayer can call this function");
         vm.prank(bridgeUser);
         l1Gateway.finalizeTransfer(address(0x101), amount, 1);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
     }
 
     // This scenario shouldn't be possible since initiateTransfer() should have prevented it.
-    function test_AmountTooSmallForFinalizationFee() public {
+    function test_FinalizeTranferAmountTooSmallForFinalizationFee() public {
         uint256 amount = 0.09 ether;
         vm.deal(address(l1Gateway), 1 ether);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
         vm.expectRevert("Amount must cover finalization fee");
         vm.prank(relayer);
         l1Gateway.finalizeTransfer(address(0x101), amount, 1);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+    }
+
+    function test_FinalizeTransferInvalidCounterpartyIdx() public {
+        uint256 amount = 0.1 ether;
+        vm.deal(address(l1Gateway), 1 ether);
+        vm.deal(relayer, 1 ether);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(relayer.balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
+        vm.expectRevert("Invalid counterparty index. Transfers must be relayed FIFO");
+        vm.prank(relayer);
+        l1Gateway.finalizeTransfer(address(0x101), amount, 2);
+
+        assertEq(address(l1Gateway).balance, 1 ether);
+        assertEq(relayer.balance, 1 ether);
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+    }
+
+    function test_FinalizeTransferWithInsufficientContractBalance() public {
+        uint256 amount = 4 ether;
+        uint256 counterpartyIdx = 1; // First transfer idx
+        vm.deal(address(l1Gateway), 0.09 ether);
+        assertEq(address(l1Gateway).balance, 0.09 ether);
+
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
+
+        vm.expectRevert("Insufficient contract balance");
+        vm.prank(relayer);
+        l1Gateway.finalizeTransfer(bridgeUser, amount, counterpartyIdx);
+
+        assertEq(l1Gateway.transferInitiatedIdx(), 0);
+        assertEq(l1Gateway.transferFinalizedIdx(), 1);
     }
 }