Skip to content

Commit

Permalink
fix pauser and payer reuse vuln
Browse files Browse the repository at this point in the history 
update testnet scripts
fix broken tests
update README
  • Loading branch information
@merklejerk
merklejerk committed Mar 8, 2024
1 parent f031c63 commit f8af414
Show file tree
Hide file tree
Showing 6 changed files with 210 additions and 106 deletions.
8 changes: 4 additions & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).

## 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.
Expand All @@ -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.

Expand All @@ -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 | [`0x4730fe5DB07c5092893B87A3Cf6740cA25ffE52A`](https://sepolia.etherscan.io/address/0x4730fe5DB07c5092893B87A3Cf6740cA25ffE52A) |

## Credits

Expand Down
22 changes: 1 addition & 21 deletions script/demo/Dummies.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(); }
}
}
144 changes: 135 additions & 9 deletions script/demo/SecretProtocol.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
)
}
}
}
67 changes: 18 additions & 49 deletions script/demo/Testnet.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -52,61 +51,31 @@ 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
);
require(honey.verifyBountyCanPay(d.bountyId(), payable(tx.origin)), '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());
}
}
Loading

0 comments on commit f8af414

Please sign in to comment.