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

Some users would be stuck on the L1 and not be able to migrate their Beans to the L2

bauchibred

Summary

As explained in the L2 migration video walkthrough provided by Beanstalk, in-order to curb the issue where not the same user controls their account on different chains so as to ensure no attacker front runs the migration by creating (a matching account on the L2 to steal the users migrated beans, Beanstalk now requires that users first sign their transactions with the wanted receiver before allowing a contract that owns deposits and bean-asset internal balances to redeem onto an address on L2.

Issue is that, Beanstalk uses EIP 712 to make and verify these signatures, which would not work completely for the intended use, considering Beanstalk expects contracts to make these signatures (not just EOAs), but EIP712 does not work well with smart contracts or wallets and as such only EOAs integrating with Beanstalk are ensured to be able to migrate, where as other (non-EOA) users are not.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L88-L114

function redeemDepositsAndInternalBalances(
address owner,
address reciever,
AccountDepositData[] calldata deposits,
AccountInternalBalance[] calldata internalBalances,
uint256 ownerRoots,
bytes32[] calldata proof,
uint256 deadline,
bytes calldata signature
) external payable fundsSafu noSupplyChange nonReentrant {
// verify deposits are valid.
// note: if the number of contracts that own deposits is small,
// deposits can be stored in bytecode rather than relying on a merkle tree.
verifyDepositsAndInternalBalances(owner, deposits, internalBalances, ownerRoots, proof);
​
// signature verification.
verifySignature(owner, reciever, deadline, signature);
​
// set deposits for `reciever`.
uint256 accountStalk;
for (uint256 i; i < deposits.length; i++) {
accountStalk += addMigratedDepositsToAccount(reciever, deposits[i]);
}
​
// set stalk for account.
setStalk(reciever, accountStalk, ownerRoots);
}

This function is called when a contract that owns bean-asset internal balances is to redeem onto an address on L2, now it queries verifySignature() that's implemented this way: https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L136-L151

function verifySignature(
address owner,
address reciever,
uint256 deadline,
bytes calldata signature
) internal view {
require(block.timestamp <= deadline, "Migration: permit expired deadline");
bytes32 structHash = keccak256(
abi.encode(REDEEM_DEPOSIT_TYPE_HASH, owner, reciever, deadline)
);
​
bytes32 hash = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(hash, signature);
require(signer == owner, "Migration: permit invalid signature");
}
​

Note that these signatures are expected to be verified for hashed data using EIP712, i.e https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L223-L226

function _hashTypedDataV4(bytes32 structHash) internal view returns (bytes32) {
return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorV4(), structHash));
}
​

Problem However is that some users would not be able to access this functionality as valid signatures from them wouldn't work since they can't work with EIP 712.

According to EIP1271: Standard Signature Validation Method for Contracts:

Externally Owned Accounts (EOA) can sign messages with their associated private keys, but currently contracts cannot. We propose a standard way for any contracts to verify whether a signature on a behalf of a given contract is valid. This is possible via the implementation of a isValidSignature(hash, signature) function on the signing contract, which can be called to validate a signature.

So while recovering a valid message signed by these set of users , the return value will be the bytes4(0) for any vote signed by a contract (e.g. Multisig) because contracts that sign messages sticking to the EIP1271 standard use the EIP1271_MAGIC_VALUE as the successful return for a properly recovered signature. A sample of this is shown within the EIP1271 and also within CompatibilityFallbackHandler by GnosisSafe.

As a result of this scenario, these set of users would not be able to migrate their Beans even though they produced valid signatures.

Impact

Some users would have their Beans stuck on the L1 even if they want to migrate to the L2 due to the current signature used with the migration logic.

Tools Used

Manual review

Recommendations

Consider adding contract signature support by implementing a recovery via the suggested isValidSignature() function of the EIP1271 and comparing the recovered value against the MAGIC_VALUE.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Multisigs will have problems migrating

Appeal created

bauchibred Submitter
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Multisigs will have problems migrating

Support

FAQs

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