From 66384e54adee62869979b4efe7a8fc147edc022f Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Mon, 7 Aug 2023 15:26:15 +0200 Subject: [PATCH 1/6] refactor: make setAuthorization idempotent --- src/Blue.sol | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Blue.sol b/src/Blue.sol index 660ffd91c..70fd572be 100644 --- a/src/Blue.sol +++ b/src/Blue.sol @@ -338,19 +338,21 @@ contract Blue is IBlue { ) external { require(block.timestamp < deadline, Errors.SIGNATURE_EXPIRED); - uint256 usedNonce = nonce[authorizer]++; + uint256 usedNonce = nonce[authorizer]; bytes32 hashStruct = keccak256(abi.encode(AUTHORIZATION_TYPEHASH, authorizer, authorized, newIsAuthorized, usedNonce, deadline)); bytes32 digest = keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, hashStruct)); address signatory = ecrecover(digest, signature.v, signature.r, signature.s); - require(signatory != address(0) && authorizer == signatory, Errors.INVALID_SIGNATURE); + if (signatory != address(0) && authorizer == signatory) { + nonce[authorizer]++; + emit IncrementNonce(msg.sender, authorizer, usedNonce); - emit IncrementNonce(msg.sender, authorizer, usedNonce); - - isAuthorized[authorizer][authorized] = newIsAuthorized; + isAuthorized[authorizer][authorized] = newIsAuthorized; + emit SetAuthorization(msg.sender, authorizer, authorized, newIsAuthorized); + } - emit SetAuthorization(msg.sender, authorizer, authorized, newIsAuthorized); + require(isAuthorized[authorizer][authorized] == newIsAuthorized, Errors.INVALID_SIGNATURE); } function setAuthorization(address authorized, bool newIsAuthorized) external { From c779a09a190a25e7fb6ecfc1e2f131c372361bcf Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Mon, 7 Aug 2023 16:09:33 +0200 Subject: [PATCH 2/6] style: comment explaining frontrun prevention --- src/Blue.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Blue.sol b/src/Blue.sol index 70fd572be..e0af8de4b 100644 --- a/src/Blue.sol +++ b/src/Blue.sol @@ -350,9 +350,10 @@ contract Blue is IBlue { isAuthorized[authorizer][authorized] = newIsAuthorized; emit SetAuthorization(msg.sender, authorizer, authorized, newIsAuthorized); + } else { + // To avoid potential frontrun attacks, only prevent changes when the signature is not verified. + require(isAuthorized[authorizer][authorized] == newIsAuthorized, Errors.INVALID_SIGNATURE); } - - require(isAuthorized[authorizer][authorized] == newIsAuthorized, Errors.INVALID_SIGNATURE); } function setAuthorization(address authorized, bool newIsAuthorized) external { From 0e56d6da5d3ea55c8619246a47ffbbe7cd0fc24d Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Mon, 7 Aug 2023 16:12:48 +0200 Subject: [PATCH 3/6] docs: clearer frontrun comment message --- src/Blue.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Blue.sol b/src/Blue.sol index e0af8de4b..9e0d5e0f9 100644 --- a/src/Blue.sol +++ b/src/Blue.sol @@ -351,7 +351,7 @@ contract Blue is IBlue { isAuthorized[authorizer][authorized] = newIsAuthorized; emit SetAuthorization(msg.sender, authorizer, authorized, newIsAuthorized); } else { - // To avoid potential frontrun attacks, only prevent changes when the signature is not verified. + // To avoid potential frontrun attacks, when the signature is not verified, only prevent attempts that actually change the authorization. require(isAuthorized[authorizer][authorized] == newIsAuthorized, Errors.INVALID_SIGNATURE); } } From 5e84d6c92afcc0970bba6cd173b281eca6a5683c Mon Sep 17 00:00:00 2001 From: MathisGD Date: Fri, 11 Aug 2023 14:07:30 +0200 Subject: [PATCH 4/6] refactor: new signature frontrun fix --- src/Blue.sol | 31 +++++++++++-------------------- src/interfaces/IBlue.sol | 16 +++++++++------- test/forge/Blue.t.sol | 6 ++---- test/forge/helpers/SigUtils.sol | 10 +--------- 4 files changed, 23 insertions(+), 40 deletions(-) diff --git a/src/Blue.sol b/src/Blue.sol index f64a38a94..1f38b4113 100644 --- a/src/Blue.sol +++ b/src/Blue.sol @@ -346,30 +346,21 @@ contract Blue is IBlue { } /// @dev The signature is malleable, but it has no impact on the security here. - function setAuthorizationWithSig( - address authorizer, - address authorized, - bool newIsAuthorized, - uint256 deadline, - Signature calldata signature - ) external { - require(block.timestamp < deadline, ErrorsLib.SIGNATURE_EXPIRED); - - uint256 usedNonce = nonce[authorizer]; - bytes32 hashStruct = - keccak256(abi.encode(AUTHORIZATION_TYPEHASH, authorizer, authorized, newIsAuthorized, usedNonce, deadline)); + function setAuthorizationWithSig(Authorization calldata authorization, Signature calldata signature) external { + require(block.timestamp < authorization.deadline, ErrorsLib.SIGNATURE_EXPIRED); + + bytes32 hashStruct = keccak256(abi.encode(AUTHORIZATION_TYPEHASH, authorization)); bytes32 digest = keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, hashStruct)); address signatory = ecrecover(digest, signature.v, signature.r, signature.s); - if (signatory != address(0) && authorizer == signatory) { - nonce[authorizer]++; - emit EventsLib.IncrementNonce(msg.sender, authorizer, usedNonce); + require(signatory != address(0) && authorization.authorizer == signatory, ErrorsLib.INVALID_SIGNATURE); - isAuthorized[authorizer][authorized] = newIsAuthorized; - emit EventsLib.SetAuthorization(msg.sender, authorizer, authorized, newIsAuthorized); - } else { - // To avoid potential frontrun attacks, when the signature is not verified, only prevent attempts that actually change the authorization. - require(isAuthorized[authorizer][authorized] == newIsAuthorized, ErrorsLib.INVALID_SIGNATURE); + if (authorization.nonce == nonce[authorization.authorizer]) { + emit EventsLib.IncrementNonce(msg.sender, authorization.authorizer, nonce[authorization.authorizer]++); + isAuthorized[authorization.authorizer][authorization.authorized] = authorization.isAuthorized; + emit EventsLib.SetAuthorization( + msg.sender, authorization.authorizer, authorization.authorized, authorization.isAuthorized + ); } } diff --git a/src/interfaces/IBlue.sol b/src/interfaces/IBlue.sol index f19f10e2e..af075c5bf 100644 --- a/src/interfaces/IBlue.sol +++ b/src/interfaces/IBlue.sol @@ -13,6 +13,14 @@ struct Market { uint256 lltv; } +struct Authorization { + address authorizer; + address authorized; + bool isAuthorized; + uint256 nonce; + uint256 deadline; +} + /// @notice Contains the `v`, `r` and `s` parameters of an ECDSA signature. struct Signature { uint8 v; @@ -62,13 +70,7 @@ interface IBlue is IFlashLender { function flashLoan(address token, uint256 amount, bytes calldata data) external; function setAuthorization(address authorized, bool newIsAuthorized) external; - function setAuthorizationWithSig( - address authorizer, - address authorized, - bool newIsAuthorized, - uint256 deadline, - Signature calldata signature - ) external; + function setAuthorizationWithSig(Authorization calldata authorization, Signature calldata signature) external; function extsload(bytes32[] memory slots) external view returns (bytes32[] memory res); } diff --git a/test/forge/Blue.t.sol b/test/forge/Blue.t.sol index 6d0cd6694..11bdacc98 100644 --- a/test/forge/Blue.t.sol +++ b/test/forge/Blue.t.sol @@ -858,7 +858,7 @@ contract BlueTest is privateKey = bound(privateKey, 1, type(uint32).max); // "Private key must be less than the secp256k1 curve order (115792089237316195423570985008687907852837564279074904382605163141518161494337)." address authorizer = vm.addr(privateKey); - SigUtils.Authorization memory authorization = SigUtils.Authorization({ + Authorization memory authorization = Authorization({ authorizer: authorizer, authorized: authorized, isAuthorized: isAuthorized, @@ -871,9 +871,7 @@ contract BlueTest is Signature memory sig; (sig.v, sig.r, sig.s) = vm.sign(privateKey, digest); - blue.setAuthorizationWithSig( - authorization.authorizer, authorization.authorized, authorization.isAuthorized, authorization.deadline, sig - ); + blue.setAuthorizationWithSig(authorization, sig); assertEq(blue.isAuthorized(authorizer, authorized), isAuthorized); assertEq(blue.nonce(authorizer), 1); diff --git a/test/forge/helpers/SigUtils.sol b/test/forge/helpers/SigUtils.sol index 7c13b2450..8071dc11d 100644 --- a/test/forge/helpers/SigUtils.sol +++ b/test/forge/helpers/SigUtils.sol @@ -1,17 +1,9 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; -import {AUTHORIZATION_TYPEHASH} from "src/Blue.sol"; +import {Authorization, AUTHORIZATION_TYPEHASH} from "src/Blue.sol"; library SigUtils { - struct Authorization { - address authorizer; - address authorized; - bool isAuthorized; - uint256 nonce; - uint256 deadline; - } - /// @dev Computes the hash of the EIP-712 encoded data. function getTypedDataHash(bytes32 domainSeparator, Authorization memory authorization) public From 5b3ac9a513bc00ac84bb960a379b219a9e6a80a9 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Fri, 11 Aug 2023 14:30:04 +0200 Subject: [PATCH 5/6] test: improve authorizationWithSig testing --- test/forge/Blue.t.sol | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/test/forge/Blue.t.sol b/test/forge/Blue.t.sol index 11bdacc98..da6bdf619 100644 --- a/test/forge/Blue.t.sol +++ b/test/forge/Blue.t.sol @@ -851,30 +851,55 @@ contract BlueTest is vm.stopPrank(); } - function testAuthorizationWithSig(uint32 deadline, address authorized, uint256 privateKey, bool isAuthorized) - public - { - deadline = uint32(bound(deadline, block.timestamp + 1, type(uint32).max)); - privateKey = bound(privateKey, 1, type(uint32).max); // "Private key must be less than the secp256k1 curve order (115792089237316195423570985008687907852837564279074904382605163141518161494337)." - address authorizer = vm.addr(privateKey); - - Authorization memory authorization = Authorization({ - authorizer: authorizer, - authorized: authorized, - isAuthorized: isAuthorized, - nonce: blue.nonce(authorizer), - deadline: block.timestamp + deadline - }); + function testAuthorizationWithSig(Authorization memory authorization, uint256 privateKey) public { + vm.assume(authorization.deadline > block.timestamp); + + // Private key must be less than the secp256k1 curve order. + privateKey = bound(privateKey, 1, type(uint32).max); + authorization.nonce = 0; + authorization.authorizer = vm.addr(privateKey); + Signature memory sig; bytes32 digest = SigUtils.getTypedDataHash(blue.DOMAIN_SEPARATOR(), authorization); + (sig.v, sig.r, sig.s) = vm.sign(privateKey, digest); + + blue.setAuthorizationWithSig(authorization, sig); + + assertEq(blue.isAuthorized(authorization.authorizer, authorization.authorized), authorization.isAuthorized); + assertEq(blue.nonce(authorization.authorizer), 1); + } + + function testAuthorizationWithSigWrongSig(Authorization memory authorization, uint256 privateKey) public { + vm.assume(authorization.deadline > block.timestamp); + + // Private key must be less than the secp256k1 curve order. + privateKey = bound(privateKey, 1, type(uint32).max); + authorization.nonce = 1; + authorization.authorizer = vm.addr(privateKey); Signature memory sig; + bytes32 digest = SigUtils.getTypedDataHash(blue.DOMAIN_SEPARATOR(), authorization); (sig.v, sig.r, sig.s) = vm.sign(privateKey, digest); blue.setAuthorizationWithSig(authorization, sig); - assertEq(blue.isAuthorized(authorizer, authorized), isAuthorized); - assertEq(blue.nonce(authorizer), 1); + assertEq(blue.isAuthorized(authorization.authorizer, authorization.authorized), false); + assertEq(blue.nonce(authorization.authorizer), 0); + } + + function testAuthorizationWithSigWrongNonce(Authorization memory authorization, uint256 privateKey) public { + vm.assume(authorization.deadline > block.timestamp); + + // Private key must be less than the secp256k1 curve order. + privateKey = bound(privateKey, 1, type(uint32).max); + authorization.nonce = 1; + + Signature memory sig; + bytes32 digest = SigUtils.getTypedDataHash(blue.DOMAIN_SEPARATOR(), authorization); + (sig.v, sig.r, sig.s) = vm.sign(privateKey, digest); + + vm.expectRevert(bytes(ErrorsLib.INVALID_SIGNATURE)); + blue.setAuthorizationWithSig(authorization, sig); } function testFlashLoan(uint256 amount) public { From 34c0bd8a7f6c818f7295231cb86c84f2e69a5d2a Mon Sep 17 00:00:00 2001 From: MathisGD Date: Fri, 11 Aug 2023 15:03:06 +0200 Subject: [PATCH 6/6] docs: authorization struct natspec --- src/interfaces/IBlue.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/interfaces/IBlue.sol b/src/interfaces/IBlue.sol index fa4881c77..051e57db1 100644 --- a/src/interfaces/IBlue.sol +++ b/src/interfaces/IBlue.sol @@ -19,6 +19,12 @@ struct Market { uint256 lltv; } +/// @notice Authorization struct. +/// @param authorizer Authorizer address. +/// @param authorized Authorized address. +/// @param isAuthorized isAuthorized to set. +/// @param nonce Signature nonce. +/// @param deadline Signature deadline. struct Authorization { address authorizer; address authorized;