hookart / protocol Goto Github PK

The constant value used to convert bips to decimals is defined as:

uint256 constant BPS_TO_DECIMAL = 10e14;

10e14 is equivalent to 1e15, which when multiplied by 10_000 bips equals 1e19, which is 10x higher than it should be.

The result is that the decimalVol and rateDecimal values calculated using this value are 10x inflated. When these values are passed into the Black Scholes algorithm, they result in incorrect bids, which automatically clear as approved by the user.

Because the volatility number increases the price of options more dramatically than the risk free rate decreases it, the result is (in most cases) a substantially increased option price.

Proof of Concept

Using the default order values provided in your test suite, we can see the difference between the correct value and the calculated value by making the _computeOptionAskAndBid() function public and running the following test:

function testZach__ConversionMakesBidTooLow() public {
        vm.warp(block.timestamp + 20 days);

        uint TIME_TO_EXPIRY = 2 weeks;
        uint VOL_DEC = 0.5e18;
        uint SPOT_DEC = 10e18;
        uint STRIKE_DEC = 12e18;
        int RATE_DEC = 0.05e18;

        (uint call,) = BlackScholes.optionPrices(
            BlackScholes.BlackScholesInputs({
                timeToExpirySec: TIME_TO_EXPIRY,
                volatilityDecimal: VOL_DEC,
                spotDecimal: SPOT_DEC,
                strikePriceDecimal: STRIKE_DEC,
                rateDecimal: RATE_DEC
            })
        );

        (, uint bid) = bidPool._computeOptionAskAndBid(
            _makeDefaultOrder(),
            _makeAssetPriceClaim(SPOT_DEC),
            block.timestamp + TIME_TO_EXPIRY,
            STRIKE_DEC,
            10 ether
        );

        console.log(call);
        console.log(bid);
    }

The result is a price over 200x higher for the same options:

Logs:
  13731454889016255
  3254808296693654060

Recommendation

-uint256 constant BPS_TO_DECIMAL = 10e14;
+uint256 constant BPS_TO_DECIMAL = 1e14;

The optionInstrumentationAddress that is inputted by the seller as an argument to the sellOption() function is used for all checks within the function. The optionMarketAddress that is a part of the signed order is ignored, and the two are not validated against one another.

Because of this insufficient parameter validation, a malicious seller can create a fake instrument with the following properties:

• getStrikePrice() returns 1 (to maximize option value)
• getExpiration() returns the maximum time the user will allow (to maximize option value)
• safeTransferFrom() does not revert

Here is an example of the simplest version of such a contract:

contract FakeInstrument {
    function getStrikePrice(uint id) public pure returns (uint) {
        return 1;
    }

    function getExpiration(uint id) public view returns (uint) {
        return block.timestamp + 79 days;
    }

    function safeTransferFrom(address from, address to, uint id) public {}
}

They can then call sellOption() with this fake instrument, and a value for saleProceeds that equals the lower of underlying asset price - fee - 1 or user's WETH balance - fee.

Let's trace the flow of this attack through the contract:

1. In _performSellOptionOrderChecks(), we grab the strike price and expiration directly from the inputted contract. We can set the expiry to ensure we pass the checks ensuring that the expiration time is between block.timestamp + order.minOptionDuration and block.timestamp + order.maxOptionDuration.

2. The strike price and expiry are used in _computeOptionAskAndBid() to determine the price. By setting the strike price to 1, Black Scholes will return a value for the option of approximately the full price of the underlying asset. We also pass the bid >= ask check, because the bid will equal the underlying asset price, while we calculated the saleProceed value to ensure that, even with the fee added, it would stay below this threshold.

3. We also call _validateOptionProperties() with this address, but we can specify the return values from calls to the contract however we want to ensure we pass the validateProperty checks.

4. Finally, we call safeTransferFrom() on our phony address, which will transfer nothing to the buyer.

5. We then transfer saleProceeds from the maker to ourselves, and the fee to the protocol. Because we set saleProceeds intentionally to ensure that the resulting value would be less than or equal to the user's WETH balance, these transactions succeed and we steal the assets from the user.

Proof of Concept

Here is a drop in test that can be added to your HookBidPoolTest.t.sol file to validate the finding.

In this case, I've assumed an underlying asset value of 50 ETH to take advantage of the user's account balance of 50 ETH, but other than that, used the original test setup and signing.

function testZach__StealWithFakeInstrument() public {
    vm.warp(block.timestamp + 20 days);
    PoolOrders.Order memory order = _makeDefaultOrder();
    (Signatures.Signature memory signature, bytes32 orderHash) = _signOrder(order, bidderPkey);

    address fakeInstrument = address(new FakeInstrument());

    console.log("Victim Starting Balance: ", weth.balanceOf(address(bidder)));
    console.log("Attacker Starting Balance: ", weth.balanceOf(address(seller)));

    vm.prank(address(seller));
    bidPool.sellOption(
        order,
        signature,
        _makeAssetPriceClaim(50 ether),
        _makeOrderClaim(orderHash),
        47.5 ether,
        fakeInstrument,
        0
    );

    console.log("Victim Ending Balance: ", weth.balanceOf(address(bidder)));
    console.log("Attacker Ending Balance: ", weth.balanceOf(address(seller)));
}

Output:

Logs:
  Victim Starting Balance:  50000000000000000000
  Attacker Starting Balance:  0
  Victim Ending Balance:  125000000000000000
  Attacker Ending Balance:  47500000000000000000

Recommendations

Verify that order.optionMarketAddress == optionInstrumentAddress for all calls to sellOption().

maxStrikePriceMultiple is defined as how many bips out of the money an option can be filled at:

/// @notice bips in the money or out of the money an option can be filled at. For example, 5000 == 50% out of the money max for a call option. 0 means no max
uint256 maxStrikePriceMultiple;

It is used in the following check:

require(
    order.maxStrikePriceMultiple == 0
        || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);

This equality assumes that maxStrikePriceMultiple is calculated in decimals.

Therefore, any value that is set in bips will be far too small, and will fail the check.

Recommendation

Change maxStrikePriceMultiple to define the decimal value, instead of the bips value.

When the EIP712 Domain Separator is calculated, we input the hookAddress and perform the following function:

function setAddressForEipDomain(address hookAddress) internal {
    // Compute `EIP712_DOMAIN_SEPARATOR`
    {
        uint256 chainId;
        assembly {
            chainId := chainid()
        }
        EIP712_DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                keccak256(
                    "EIP712Domain(" "string name," "string version," "uint256 chainId," "address verifyingContract"
                    ")"
                ),
                keccak256("Hook"),
                keccak256("1.0.0"),
                chainId,
                hookAddress
            )
        );
    }
}

This correctly uses the name, version, chain id, and verifying contract to generator a domain separator to be used for all EIP 712 encoding.

However, if we look at HookBidPool.sol at where this function is being called, we see the following:

constructor(..., address _protocol) {
    ...
    setAddressForEipDomain(_protocol);
    ...
}

function setProtocol(address _protocol) external onlyRole(PROTOCOL_ROLE) {
    setAddressForEipDomain(_protocol);
    protocol = IHookProtocol(_protocol);
    emit ProtocolAddressSet(_protocol);
}

These two function use the Hook protocol address to create the domain. However, EIP712 advises that you use the verifying contract itself (in other words, HookBidPool.sol for this value).

There are two reasons for this:

1. If you implement signature checks elsewhere within the protocol, it could result in signature collision and potential replay attacks.

2. If you change the protocol address and update it using setProtocol(), all previously signed bids will become invalid.

Recommendation

Set the address for EIP712 Domain Separator to address(this) in the constructor, and set the variable to immutable so that it cannot be changed in the future.

_performSellOptionOrderChecks() performs the following two checks:

require(expiry > block.timestamp, "Option is expired");
require(block.timestamp + order.minOptionDuration < expiry, "Option is too close to expiry");

Since order.minOptionDuration is unsigned, it is necessarily the case that block.timestamp + order.minOptionDuration >= block.timestamp.

Therefore, if the second check passes, it is necessarily true that the first check would have passed as well.

We can save some gas by removing the first check.

Recommendations

-require(expiry > block.timestamp, "Option is expired");
 require(block.timestamp + order.minOptionDuration < expiry, "Option is too close to expiry");

The priceOracleSigner is set using the following admin-only function:

function setPriceOracleSigner(address _priceOracleSigner) external onlyRole(ORACLE_ROLE) {
    priceOracleSigner = _priceOracleSigner;
    emit PriceOracleSignerUpdated(_priceOracleSigner);
}

Similarly, the orderValidityOracleSigner is set with this function:

function setOrderValidityOracleSigner(address _orderValidityOracleSigner) external onlyRole(ORACLE_ROLE) {
    orderValidityOracleSigner = _orderValidityOracleSigner;
    emit OrderValidityOracleSignerUpdated(_orderValidityOracleSigner);
}

There are no checks on what either of these values are set to.

While I'm not usually a fan of zero address checks (because in most cases, the zero address would be no worse than setting an arbitrary other un-owned address), in this case, it is important.

This is because, later, asset price claims are validated as follows:

function _validateAssetPriceClaim(AssetPriceClaim calldata claim) internal view {
    bytes memory claimEncoded =
        abi.encode(claim.assetPriceInWei, claim.priceObservedTimestamp, claim.goodTilTimestamp);

    bytes32 claimHash = keccak256(claimEncoded);
    bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", claimHash));

    address signer = ecrecover(prefixedHash, claim.v, claim.r, claim.s);

    require(signer == priceOracleSigner, "Claim is not signed by the priceOracle");
    require(claim.goodTilTimestamp > block.timestamp, "Claim is expired");
}

And whether the validity oracle signed is validated as follows:

function _validateOrderValidityOracleClaim(OrderValidityOracleClaim calldata claim, bytes32 orderHash)
    internal
    view
{
    bytes memory claimEncoded = abi.encode(orderHash, claim.goodTilTimestamp);

    bytes32 claimHash = keccak256(claimEncoded);
    bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", claimHash));

    address signer = ecrecover(prefixedHash, claim.v, claim.r, claim.s);

    require(signer == orderValidityOracleSigner, "Claim is not signed by the orderValidityOracle");
    require(claim.goodTilTimestamp > block.timestamp, "Claim is expired");
}

Both of these functions check whether the return value of ecrecover matches with the oracle's address. However, any malicious user is able to create a fraudulent signature which returns address(0) from ecrecover. See this example for how this can be done.

In the case that orderValidityOracleSigner is set to address(0), an attacker could execute orders that have been cancelled off-chain.

In the case that priceOracleSigner is set to address(0), an attacker could drain the accounts of any user with open bids by submitting extremely high asset values for the assets they've made offers on, leading to arbitrarily high bid prices.

Recommendation

Check when these oracles are being set to confirm that the new value is not address(0).

Alternatively, you could verify in the _validateAssetPriceClaim() and _validateOrderValidityOracleClaim() functions that signer != address(0). However, I would recommend the former solution, as we'd rather keep the gas used in common functions as low as possible.

When comparing order.maxStrikePriceMultiple to the actual multiple by which the strike price exceeds the asset price, the calculation is performed as follows:

require(
    order.maxStrikePriceMultiple == 0
        || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);

As we can see, the difference in price is multiplied by 10e18 before being divided by the current price.

10e18 is equivalent to 1e19, which is 10x higher than the intended value.

The result is that the multiple representing the gap between the strike price and asset price will always be overrepresented by 10x, causing it to fail the comparison to the maxStrikePriceMultiple.

Recommendation

require(
    order.maxStrikePriceMultiple == 0
-       || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
+       || (strikePrice - assetPrice.assetPriceInWei) * 1e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);

getPoolOrderStructHash() encodes and hashes an order to create the structHash for EIP 712 signing:

function getPoolOrderStructHash(Order memory poolOrder) internal view returns (bytes32) {
    return keccak256(abi.encodePacked(_hashPt1(poolOrder), _hashPt2(poolOrder)));
}

Since _ORDER_TYPEHASH and _PROPERTY_TYPEHASH are constants, this function does not read from storage.

It can therefore be changed from view to pure to save on gas.

Recommendation

-function getPoolOrderStructHash(Order memory poolOrder) internal view returns (bytes32) {
+function getPoolOrderStructHash(Order memory poolOrder) internal pure returns (bytes32) {
     return keccak256(abi.encodePacked(_hashPt1(poolOrder), _hashPt2(poolOrder)));
 }

The protocol uses two off chain oracles to (a) verify asset prices and (b) allow gasless cancellations.

Each of these off chain signers pose a centralization risk for the protocol:

• priceOracleSigner: This signer submits the asset prices used in the Black Scholes calculation for option value. If a malicious actor were to get control of this wallet, they could drain all the funds from every the wallet of every user with open bids by submitting high spot price values that would push the option value up.

• orderValidityOracleSigner: This signer submits confirmation that the buyer has not gaslessly cancelled their order, and it is therefore valid to execute. If a malicious actor were to get control of this wallet, they could execute cancelled transactions, forcing buyers to buy assets they did not intend to.

Both off chain signers have their risks, but the orderValidityOracleSigner seems to be accomplishing an important goal (gasless cancellations) and the downsides are limited: orders that have no yet expired can be executed within the originally defined bounds, based on the accurate asset price.

The priceOracleSigner, on the other hand, seems to create an undue risk for users.

Recommendation

For orderValidityOracleSigner, consider whether the feature of gasless cancellations is worth the key compromise risk.

For priceOracleSigner, it is recommended to use a reputable, decentralized oracle such as Chainlink for such an important source of data. Unfortunately, Chainlink's NFT Floor Price feeds are limited to only 10 NFTs at the moment, so I understand that this would pose a major trade off for the protocol.

HookBidPool defines four roles: ORACLE_ROLE, PAUSER_ROLE, PROTOCOL_ROLE, and FEES_ROLE. Each of these is set to the _initialAdmin in the constructor, with plans to transfer the roles to separate wallets:

_grantRole(ORACLE_ROLE, _initialAdmin);
_setRoleAdmin(ORACLE_ROLE, ORACLE_ROLE);
_grantRole(PAUSER_ROLE, _initialAdmin);
_setRoleAdmin(PAUSER_ROLE, PAUSER_ROLE);
_grantRole(PROTOCOL_ROLE, _initialAdmin);
_setRoleAdmin(PROTOCOL_ROLE, PROTOCOL_ROLE);
_grantRole(FEES_ROLE, _initialAdmin);
_setRoleAdmin(FEES_ROLE, FEES_ROLE);

The code above grants each of these roles to _initialAdmin, and also sets the role's admin to self.

This pattern is not advised, and is only recommended to be used for one master admin role with special safety precautions taken. This is because the pattern can result in permanently losing access to functions gated to a specific role if that role is ever renounced or revoked.

Instead, it is recommended to use the DEFAULT_ADMIN_ROLE as the admin for for these different roles, and to grant this role to highly trusted addresses (ideally with some redundancy in case keys are lost).

Recommendation

Start off by granting the DEFAULT_ADMIN_ROLE to the _initialAdmin:

_grantRole(ORACLE_ROLE, _initialAdmin);
_grantRole(PAUSER_ROLE, _initialAdmin);
_grantRole(PROTOCOL_ROLE, _initialAdmin);
_grantRole(FEES_ROLE, _initialAdmin);
_grantRole(DEFAULT_ADMIN_ROLE, _initialAdmin);

Then you can proceed to grant the admin role to trusted multisigs or the DAO governance address for maximum safety.

In both _validateOrderValidityOracleClaim() and _validateAssetPriceClaim(), we check the message that has been signed by the off-chain oracle by encoding the data as follows:

bytes32 claimHash = keccak256(claimEncoded);
bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", claimHash));

This does not follow the format laid out in EIP 191, which specifies that the message following the prefix should be len(message) ‖ message and not keccak(message.

While it seems that the prefix is only being used for ease of testing, it is important not to mix and match signature standards. If you use the EIP 191 standard for signed messages, it is advised to follow it as written.

Recommendation

Encode the data as follows to follow the EIP 191 standard:

bytes32 claimLen = len(claimEncoded);
bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", claimLen, claimEncoded));