0x52 - resolveQueuedTrades is intended to be non atomic but invalid signature can still cause entire transaction to revert

[sherlock-admin]

0x52

medium

resolveQueuedTrades is intended to be non atomic but invalid signature can still cause entire transaction to revert

Summary

BufferRouter#resolveQueuedTrades and unlockOptions attempt to be non atomic (i.e. doesn't revert the transaction if one fails) but an invalid signature can still cause the entire transaction to revert, because the ECDSA.recover sub call in _validateSigner can still revert.

Vulnerability Detail

function _validateSigner(
    uint256 timestamp,
    address asset,
    uint256 price,
    bytes memory signature
) internal view returns (bool) {
    bytes32 digest = ECDSA.toEthSignedMessageHash(
        keccak256(abi.encodePacked(timestamp, asset, price))
    );
    address recoveredSigner = ECDSA.recover(digest, signature);
    return recoveredSigner == publisher;
}

_validateSigner can revert at the ECDSA.recover sub call breaking the intended non atomic nature of BufferRouter#resolveQueuedTrades and unlockOptions.

Impact

BufferRouter#resolveQueuedTrades and unlockOptions don't function as intended if signature is malformed

Code Snippet

https://github.com/sherlock-audit/2022-11-buffer/blob/main/contracts/contracts/core/BufferRouter.sol#L260-L271

Tool used

Manual Review

Recommendation

Use a try statement inside _validateSigner to avoid any reverts:

    function _validateSigner(
        uint256 timestamp,
        address asset,
        uint256 price,
        bytes memory signature
    ) internal view returns (bool) {
        bytes32 digest = ECDSA.toEthSignedMessageHash(
            keccak256(abi.encodePacked(timestamp, asset, price))
        );
-       address recoveredSigner = ECDSA.recover(digest, signature);

+       try ECDSA.recover(digest, signature) returns (address recoveredSigner) {
+           return recoveredSigner == publisher;
+       } else {
+           return false;
+       }
    }

[bufferfinance]

The protocol has been tested against wrong signatures.
https://github.com/sherlock-audit/2022-11-buffer/blob/main/contracts/tests/test_router.py#L815

[IAm0x52]

Escalate for 10 USDC.

My submission is valid and sponsor's comment here is inaccurate. ECDSA.recover will revert in the _throwError subcall under quite a few conditions not covered by their tests, including signature of invalid length and signature that resolve to address(0).

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/24d1bb668a1152528a6e6d71c2e285d227ed19d9/contracts/utils/cryptography/ECDSA.sol#L88-L92

[sherlock-admin]

Escalate for 10 USDC.

My submission is valid and sponsor's comment here is inaccurate. ECDSA.recover will revert in the _throwError subcall under quite a few conditions not covered by their tests, including signature of invalid length and signature that resolve to address(0).

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/24d1bb668a1152528a6e6d71c2e285d227ed19d9/contracts/utils/cryptography/ECDSA.sol#L88-L92

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Invalid signatures resolving to address(0) reverts _validateSigner

[sherlock-admin]

Escalation accepted

Invalid signatures resolving to address(0) reverts _validateSigner

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[bufferfinance]

Will fix this

[IAm0x52]

Fixed in PR#28

Changes look good. ECDSA.recover changed to ECDSA.tryRecover to prevent any revert when recovering signatures