Identify bounty in payer and pauser callbacks. (#3)

* fix pauser and payer reuse vuln update testnet scripts fix broken tests update README * update readme * Add Test to Validate Mitigation of Unauthorized Bounty Claim (#5) Added test to check for bounty using another pauser payer * Return pay amount (#4) * return paid amount from claims * update readme * update flow img * update flow img --------- Co-authored-by: JordanCason <[email protected]>
merklejerk · Mar 9, 2024 · 50888b9 · 50888b9
1 parent f031c63
commit 50888b9
Show file tree

Hide file tree

Showing 8 changed files with 358 additions and 138 deletions.
diff --git a/README.md b/README.md
@@ -13,7 +13,7 @@ Projects register bounties with the HoneyPause contract via the `add()` function
 1. A bounty token and amount, but no deposit*.
 2. The address of a custom [`Verifier`](ause/blob/main/src/HoneyPause.sol#L8) contract which asserts onchain state invariants that would be violated in the case of an exploit. Examples could be an AMM's reserve violating the constant product formula or a lending protocol incurring bad debt.
 3. The address of a custom [`Pauser`](./src/HoneyPause.sol#L28) contract which is authorized to pause/freeze the protocol when called by the HoneyPause contract.
-4. The address of a custom [`Payer`](./src/HoneyPause.sol#L42) contract which must pay the bounty to the whitehat when called by the HoneyPause contract. 
+4. The address of a custom [`Payer`](./src/HoneyPause.sol#L44) contract which must pay the bounty to the whitehat when called by the HoneyPause contract. 
 5. An operator account for the bounty, who will be able to modify the bounty.
 
 > \* Note that the HoneyPause contract never custodies bounties. It is up to the project's **Payer** contract to surface funds to cover the bounty when called.
@@ -36,7 +36,7 @@ A whitehat that has discovered an exploit on a registered project will submit a
 
 > ⚠️ On Ethereum mainnet, it is critical that the whitehat uses a private mempool mechanism (e.g., Flashbots Protect with max privacy) to submit the transaction in order to prevent discovery of the exploit mechanism before the transaction is mined and the protocol can be paused! On other chains where sequencing cannot be practically frontrun, it may be sufficient to submit directly to the tx sequencer.
 
-You can check out an example trace of a claim tx [here](https://phalcon.blocksec.com/explorer/tx/sepolia/0xd3ce2ef3a80a6461142020909acc8499e8b6e893073c77d534734d7d129abdc7).
+You can check out an example trace of a claim tx [here](https://phalcon.blocksec.com/explorer/tx/sepolia/0x1b957becc3839f34d1e7f221eeb7353e6073b69da91ac717478a64c3fd2bf291) (older version).
 
 ## Writing Verifiers
 **Verifier**s must confirm that some critical invariants or health checks have been violated in the post-exploit state. Projects need to do the legwork of identifying a robust set of checks that would be considered critical enough to warrant pausing the entire protocol. These would typically be invariants that do not get checked during normal user interactions due to gas constraints.
@@ -56,10 +56,10 @@ A **Verifier**'s `beforeExploit()` function returns arbitrary data. This will la
 If the **Verifier** applies any state changes (even transient ones), they should restrict the caller to the HoneyPause contract. Otherwise the **Verifier** may be maliciously invoked before the call to `claim()` to manipulate results.
 
 ## Writing Pausers
-Because the exploit will be detailed onchain for all to see after the claim tx is made, **Pausers** should pause as much of the protocol to prevent replicating the exploit across related components (pools) of the system. Only the HoneyPause contract should be allowed to call `pause()` on the **Pauser** contract. The pause *must* occur when `Pauser.pause()` is called, and not in the payer, which is called immediately afterwards.
+Because the exploit will be detailed onchain for all to see after the claim tx is made, **Pausers** should pause as much of the protocol to prevent replicating the exploit across related components (pools) of the system. Only the HoneyPause contract should be allowed to call `pause()` on the **Pauser** contract and the `bountyId` parameter should be confirmed to be one created by the project. The pause *must* occur when `Pauser.pause()` is called, and not in the payer, which is called immediately afterwards.
 
 ## Writing Payers
-The **Payer** contract will be invoked by HoneyPause to transfer the bounty to the whitehat. Bounties can be in either ETH or ERC20. HoneyPause will surround the `payExploiter()` call with balance checks to ensure that payment has been delivered. The **Payer** contract should only allow the HoneyPause contract to call its `payExploiter()` function.
+The **Payer** contract will be invoked by HoneyPause to transfer the bounty to the whitehat. Bounties can be in either ETH or ERC20. HoneyPause will surround the `payExploiter()` call with balance checks to ensure that payment has been delivered. The **Payer** contract should only allow the HoneyPause contract to call its `payExploiter()` function and the `bountyId` parameter should be confirmed to be one created by the project.
 
 The simplest **Payer** implementation will transfer the bounty directly out of a dedicated fund. Alternatively, a project may choose to keep the bounty value in its protocol and perform the conversion on-the-fly. In that case, the payment mechanism should be distinct from normal user operations on the protocol because the **Payer** will be invoked *after* the **Pauser**. Keep in mind that more complex payment mechanisms can open projects up to a secondary exploit.
 
@@ -68,7 +68,7 @@ The simplest **Payer** implementation will transfer the bounty directly out of a
 | Chain | Address |
 |-------|---------|
 | Ethereum Mainnet | `TBD` |
-| Ethereum Sepolia | [`0x5cd701310ae6e3185C29de433019C96efd298d60`](https://sepolia.etherscan.io/address/0x5cd701310ae6e3185c29de433019c96efd298d60) |
+| Ethereum Sepolia | [`0x81cb0ba5E7B724B03C8B64A5FAF4f2C655aD15E4`](https://sepolia.etherscan.io/address/0x81cb0ba5E7B724B03C8B64A5FAF4f2C655aD15E4) |
 
 ## Credits
 

diff --git a/assets/flow.jpg b/assets/flow.jpg
diff --git a/script/demo/Dummies.sol b/script/demo/Dummies.sol
@@ -4,18 +4,6 @@ pragma solidity ^0.8.23;
 import { ERC20 } from 'solmate/tokens/ERC20.sol';
 import { HoneyPause, IVerifier, IPauser, IPayer, IExploiter, ETH_TOKEN } from '../../src/HoneyPause.sol';
 
-contract TestPayer is IPayer {
-    constructor() payable {}
-
-    function payExploiter(ERC20 token, address payable to, uint256 amount) external {
-        if (token == ETH_TOKEN) {
-            to.transfer(amount);
-        } else {
-            token.transfer(to, amount);
-        }
-    }
-}
-
 contract TestToken is ERC20 {
     constructor(string memory name, string memory symbol, uint8 decimals)
         ERC20(name, symbol, decimals)
@@ -24,12 +12,4 @@ contract TestToken is ERC20 {
     function mint(address to, uint256 amount) external {
         _mint(to, amount);
     }
-}
-
-contract SucceedingContract {
-    fallback() external payable {}
-}
-
-contract FailingContract {
-    fallback() external payable { revert(); }
-}
+}
diff --git a/script/demo/SecretProtocol.sol b/script/demo/SecretProtocol.sol
@@ -2,24 +2,25 @@
 pragma solidity ^0.8.23;
 
 import { ERC20 } from 'solmate/tokens/ERC20.sol';
+import { TestToken } from './Dummies.sol';
 import { HoneyPause, IVerifier, IPauser, IPayer, IExploiter, ETH_TOKEN } from '../../src/HoneyPause.sol';
 
 contract SecretProtocol {
     address public immutable pauser;
     bytes3 public hash;
     bool public paused;
 
-    event PreimageFound(bytes32 preimage);
+    event PreimageFound(string preimage);
     event Paused();
 
     constructor(bytes3 hash_, address pauser_) {
         hash = hash_;
         pauser = pauser_;
     }
 
-    function solve(bytes32 preimage) external {
+    function solve(string memory preimage) external {
         require(!paused, 'paused');
-        if (hash != 0 && bytes3(keccak256(abi.encode(preimage))) == hash) {
+        if (hash != 0 && bytes3(keccak256(bytes(preimage))) == hash) {
             emit PreimageFound(preimage);
             hash = 0;
         }
@@ -51,36 +52,161 @@ contract SecretProtocolVerifier is IVerifier {
 
 contract SecretExploiter is IExploiter {
     function exploit(bytes memory exploiterData) external {
-        (SecretProtocol proto, bytes32 preimage) =
-            abi.decode(exploiterData, (SecretProtocol, bytes32));
+        (SecretProtocol proto, string memory preimage) =
+            abi.decode(exploiterData, (SecretProtocol, string));
         proto.solve(preimage);
     }
 }
 
 contract SecretProtocolPauser is IPauser {
     HoneyPause immutable honey;
     SecretProtocol public immutable proto;
+    uint256 immutable public bountyId;
 
-    constructor(HoneyPause honey_, SecretProtocol proto_) {
+    constructor(
+        SecretProtocol proto_,
+        HoneyPause honey_,
+        uint256 bountyId_
+    ) {
         honey = honey_;
         proto = proto_;
+        bountyId = bountyId_;
     }
 
-    function pause() external {
+    function pause(uint256 bountyId_) external {
         require(msg.sender == address(honey), 'not honey');
+        require(bountyId_ == bountyId, 'wrong bounty');
         proto.pause();
     }
 }
 
 contract SecretProtocolPayer is IPayer {
     HoneyPause immutable honey;
+    uint256 immutable bountyId;
 
-    constructor(HoneyPause honey_) {
+    constructor(HoneyPause honey_, uint256 bountyId_) {
         honey = honey_;
+        bountyId = bountyId_;
     }
 
-    function payExploiter(ERC20 token, address payable to, uint256 amount) external {
+    function payExploiter(
+        uint256 bountyId_,
+        ERC20 token,
+        address payable to,
+        uint256 amount
+    )
+        external
+    {
         require(msg.sender == address(honey), 'not honey');
+        require(bountyId_ == bountyId, 'wrong bounty');
         token.transfer(to, amount);
     }
+}
+
+contract SecretProtocolBountyDeployer {
+    event Deployed(
+        uint256 bountyId,
+        SecretProtocol proto,
+        SecretProtocolPauser pauser,
+        SecretProtocolVerifier verifier,
+        SecretProtocolPayer payer
+    );
+
+    SecretProtocol immutable public proto;
+    SecretProtocolPauser immutable public pauser;
+    SecretProtocolVerifier immutable public verifier;
+    SecretProtocolPayer immutable public payer;
+    uint256 immutable public bountyId;
+
+    constructor(
+        HoneyPause honey,
+        string memory name,
+        TestToken token,
+        uint256 amount,
+        bytes3 hash,
+        address operator
+    ) {
+        bountyId = honey.bountyCount() + 1;
+        address pauserAddress = _getDeployedAddress(address(this), 2);
+        proto = new SecretProtocol(hash, pauserAddress);
+        pauser = new SecretProtocolPauser(proto, honey, bountyId);
+        assert(address(pauser) == pauserAddress);
+        verifier = new SecretProtocolVerifier(proto);
+        payer = new SecretProtocolPayer(honey, bountyId);
+        token.mint(address(payer), amount);
+        uint256 bountyId_ = honey.add({
+            name: name,
+            payoutToken: token,
+            payoutAmount: amount,
+            verifier: verifier,
+            pauser: pauser,
+            payer: payer, 
+            operator: operator
+        });
+        assert(bountyId_ == bountyId);
+        emit Deployed(bountyId, proto, pauser, verifier, payer);
+    }
+
+    function _getDeployedAddress(address deployer, uint32 deployNonce)
+        private
+        pure
+        returns (address deployed)
+    {
+        assembly {
+            mstore(0x02, shl(96, deployer))
+            let rlpNonceLength
+            switch gt(deployNonce, 0xFFFFFF)
+                case 1 { // 4 byte nonce
+                    rlpNonceLength := 5
+                    mstore8(0x00, 0xD8)
+                    mstore8(0x16, 0x84)
+                    mstore(0x17, shl(224, deployNonce))
+                }
+                default {
+                    switch gt(deployNonce, 0xFFFF)
+                        case 1 {
+                            // 3 byte nonce
+                            rlpNonceLength := 4
+                            mstore8(0x16, 0x83)
+                            mstore(0x17, shl(232, deployNonce))
+                        }
+                        default {
+                            switch gt(deployNonce, 0xFF)
+                                case 1 {
+                                    // 2 byte nonce
+                                    rlpNonceLength := 3
+                                    mstore8(0x16, 0x82)
+                                    mstore(0x17, shl(240, deployNonce))
+                                }
+                                default {
+                                    switch gt(deployNonce, 0x7F)
+                                        case 1 {
+                                            // 1 byte nonce >= 0x80
+                                            rlpNonceLength := 2
+                                            mstore8(0x16, 0x81)
+                                            mstore8(0x17, deployNonce)
+                                        }
+                                        default {
+                                            rlpNonceLength := 1
+                                            switch iszero(deployNonce)
+                                                case 1 {
+                                                    // zero nonce
+                                                    mstore8(0x16, 0x80)
+                                                }
+                                                default {
+                                                    // 1 byte nonce < 0x80
+                                                    mstore8(0x16, deployNonce)
+                                                }
+                                        }
+                                }
+                        }
+                }
+            mstore8(0x00, add(0xD5, rlpNonceLength))
+            mstore8(0x01, 0x94)
+            deployed := and(
+                keccak256(0x00, add(0x16, rlpNonceLength)),
+                0xffffffffffffffffffffffffffffffffffffffff
+            )
+        }
+    }
 }
diff --git a/script/demo/Testnet.s.sol b/script/demo/Testnet.s.sol
@@ -6,12 +6,11 @@ import { ERC20 } from 'solmate/tokens/ERC20.sol';
 import { HoneyPause, IVerifier, IPauser, IPayer, ETH_TOKEN } from '../../src/HoneyPause.sol';
 import {
     SecretProtocol,
-    SecretProtocolVerifier,
     SecretExploiter,
-    SecretProtocolPauser,
-    SecretProtocolPayer
+    SecretProtocolBountyDeployer,
+    SecretProtocolVerifier
 } from './SecretProtocol.sol';
-import { TestPayer, TestToken, SucceedingContract, FailingContract } from './Dummies.sol';
+import { TestToken } from './Dummies.sol';
 
 contract Testnet is Script {
     uint256 deployerKey;
@@ -52,61 +51,32 @@ contract Testnet is Script {
         new TestToken('USDC', 'USDC', 6);
     }
 
-    function exploitProtocol(uint256 bountyId, SecretProtocol proto, bytes32 preimage) external {
-        require(bytes3(keccak256(abi.encode(preimage))) == proto.hash(), 'invalid preimage');
+    function exploitProtocol(uint256 bountyId, string memory preimage) external {
+        (,,, IVerifier verifier,,) = honey.getBounty(bountyId);
+        SecretProtocol proto = SecretProtocolVerifier(address(verifier)).proto();
+        require(bytes3(keccak256(bytes(preimage))) == proto.hash(), 'invalid preimage');
         _broadcast();
         SecretExploiter exploiter = new SecretExploiter();
         _broadcast();
         honey.claim(bountyId, payable(tx.origin), exploiter, abi.encode(proto, preimage), "");
     }
 
-    function registerUsdcProtocol(string memory name, uint256 amount, bytes3 hash) external {
-        address pauserAddress = vm.computeCreateAddress(deployer, vm.getNonce(deployer) + 2);
+    function registerUsdcProtocol(string memory name, uint256 amount, string memory preimage) external {
+        bytes3 hash = bytes3(keccak256(bytes(preimage)));
         _broadcast();
-        SecretProtocol proto = new SecretProtocol(hash, pauserAddress);
-        _broadcast();
-        IVerifier verifier = new SecretProtocolVerifier(proto);
-        _broadcast();
-        IPauser pauser = new SecretProtocolPauser(honey, proto);
-        assert(address(pauser) == pauserAddress);
-        _broadcast();
-        IPayer payer = new SecretProtocolPayer(honey);
-        _broadcast();
-        usdc.mint(address(payer), amount);
-        _broadcast();
-        honey.add({
-            name: name,
-            payoutToken: usdc,
-            payoutAmount: amount,
-            verifier: verifier,
-            pauser: pauser,
-            payer: payer, 
-            operator: operator
-        });
+        SecretProtocolBountyDeployer d = new SecretProtocolBountyDeployer(
+            honey,
+            name,
+            usdc,
+            amount,
+            hash,
+            operator
+        );
+        (bool canPay,) = honey.verifyBountyCanPay(d.bountyId(), payable(tx.origin));
+        require(canPay, 'cannot pay');
     }
 
     function _broadcast() private {
         vm.broadcast(deployerKey);
     }
-
-    function _deployPayerContract(ERC20 token, uint256 amount) private returns (IPayer) {
-        _broadcast();
-        if (token == ETH_TOKEN) {
-            return new TestPayer{value: amount}();
-        }
-        IPayer payer = new TestPayer();
-        _broadcast();
-        TestToken(address(token)).mint(address(payer), amount);
-        return payer;
-    }
-
-    function _deploySucceedingContract() private returns (address) {
-        _broadcast();
-        return address(new SucceedingContract());
-    }
-
-    function _deployFailingContract() private returns (address) {
-        _broadcast();
-        return address(new FailingContract());
-    }
 }