Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

[M-1] Tractor not compliant to EIP712 requirement

mattjenkins

  • Likelihood: High

  • Impact: Medium (Can still generate incorrect but working signatures)

Summary

Incorrect hash value used in the LibTractor.sol library in the _domainSeparatorV4 function which means the hash is not a standard EIP712 hash for signing. The function is using the type hash of the blueprint, not the EIP712Domain. Signature validation is performed against this incorrect domain.

Vulnerability Details

RFC #734 states : " A Blueprint Hash must be hashed following the EIP-712 standard"

In the _domainSeparatorV4 the BLUEPRINT_TYPE_HASH is used instead of EIP712_TYPE_HASH. This function is then used for both for signing in the helper function and TractorFacet also uses this incorrect function to validate the signature for a requisition, meaning that not only is there a helper function generating non compliant hashes, the validation of the hash and signature when trying to run a blueprint means all correctly formed hashes and their signatures will fail.

Also, there are no helper functions to generate the composite parts of the EIP712 hash, so as written there is no assisted way to create a eth_signTypedData with valid EIP712 signatures outside the contract.

Currently the tests also use the incorrect helper utility to generate signatures, so the tests all pass, irrespective of what value is used in the domain separator. This function can return ANY value and all the tests will still pass. To expose the problem, the tests need to generate the signature independently from the TractorLib library.

An outline of a forge based test is below which creates the signature outside the function being tested:

struct Blueprint {
address publisher;
bytes data;
bytes32[] operatorPasteInstrs;
uint256 maxNonce;
uint256 startTime;
uint256 endTime;
}
bytes32 private constant EIP712DOMAIN_TYPEHASH =
keccak256(
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
);
bytes32 public constant BLUEPRINT_TYPE_HASH =
keccak256(
"Blueprint(address publisher,bytes data,bytes operatorData,uint256 maxNonce,uint256 startTime,uint256 endTime)"
);
​
bytes32 constant domainSeparatorHash = keccak256(abi.encode(
EIP712DOMAIN_TYPEHASH,
keccak256(bytes("Tractor")),
keccak256(bytes("2")),
block.chainid,
address(<CONTRACT-ADDRESS>)
));
Blueprint constant blueprint = Blueprint(<BLUEPRINT DATA>);
​
function test_GenerateHash() public {
bytes32 dataEncoded = keccak256(
abi.encode(
BLUEPRINT_TYPE_HASH,
blueprint.publisher,
keccak256(blueprint.data),
keccak256(abi.encodePacked(blueprint.operatorPasteInstrs)),
blueprint.maxNonce,
blueprint.startTime,
blueprint.endTime
);
​
bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparatorHash, dataEncoded));
​
(uint8 v, bytes32 r, bytes32 s) = vm.sign(<PRODUCER_PRIVATE_KEY>, digest);
bytes memory signature = abi.encodePacked(r,s,v);
.....

Impact

The system as developed does not allow for the creation of correct off-chain typed signed hashes. Any producer who creates a valid EIP712 signature off chain for a blueprint/requisition will find that this cannot be run via Tractor.

There is also no way to allow a useful message to be displayed to the producer for signing from the helpful functions, as all we can get out of the TractorFacet is a fully hashed non-compliant value and also not the constituent parts required for signing typed data.

Tools Used

Foundry, Hardhat, Manual Review

Recommendations

One way to fix this possibility of human error is to use the OpenZeppelin EIP712.sol contract to wrap all the EIP712 specifics so that the protocol is consistent in its use and leverages openzeppelin which is consistent with other areas of the beanstalk protocol.

To fix the existing code, see below.

Firstly, change LibTractor.sol on line 177:

function _domainSeparatorV4() internal view returns (bytes32) {
return
keccak256(
abi.encode(
- BLUEPRINT_TYPE_HASH,
+ EIP712_TYPE_HASH,
TRACTOR_HASHED_NAME,
TRACTOR_HASHED_VERSION,
C.getChainId(),
address(this)
)
);
}

This at least creates valid EIP712 hashes.

To ensure that the helper functions actually allow for the intended use of EIP712 in terms of showing useful messages to Producers creating blueprints, the protocol should expose additional functions to create the Blueprint hash and DomainSeparator hash separately.

An example of changes to LibTractor and TractorFacet to aid in providing better suite of helper functions would be:

LibTractor:

+ function _getBlueprintHashOnly(Blueprint calldata blueprint) internal view returns (bytes32) {
+ return
+ keccak256(
+ abi.encode(
+ BLUEPRINT_TYPE_HASH,
+ blueprint.publisher,
+ keccak256(blueprint.data),
+ keccak256(abi.encodePacked(blueprint.operatorPasteInstrs)),
+ blueprint.maxNonce,
+ blueprint.startTime,
+ blueprint.endTime
+ )
+ );
+ }

and then for TractorFacet.sol:

function getBlueprintHash(
LibTractor.Blueprint calldata blueprint
) external view returns (bytes32) {
return LibTractor._getBlueprintHashOnly(blueprint);
}
​
function getFullEIP712Hash(
LibTractor.Blueprint calldata blueprint
) external view returns (bytes32) {
return LibTractor._getBlueprintHash(blueprint);
}
​
function getDomainSeparatorOnly(
) external view returns (bytes32) {
return LibTractor._domainSeparatorV4();
}

These can then be used on the client side to send legible messages for signing.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Tractor not compliant to EIP712 requirement because it's using the type hash of the blueprint, not the EIP712Domain

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.