084.md

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;
+       }
    }