Summary
Nonces not used in signed data causing replay attacks
Vulnerability Details
Impact
A nonce can prevent an old value from being used when a new value exists. Without one, two transactions submitted in one order, can appear in a block in a different order
In ProxyFactory.sol, deployProxyAndDistributeBySignature() is used to deploy proxy contract and distribute prize by signature.
function deployProxyAndDistributeBySignature(
address organizer,
bytes32 contestId,
address implementation,
bytes calldata signature,
bytes calldata data
) public returns (address) {
>> bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
if (ECDSA.recover(digest, signature) != organizer) revert ProxyFactory__InvalidSignature();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
address proxy = _deployProxy(organizer, contestId, implementation);
_distribute(proxy, data);
return proxy;
}
The function uses EIP712 to verify the signature to avoid replay attacks but the signed data does not contain the nonce which causes replay attacks. This implementation is also against the security recommendation of EIP-712.
Since deployProxyAndDistributeBySignature() is used on behalf of organizer. If a deployer is attacked, then tries to change the function arguments if he initially chooses an incorrect one, but he immediately notices the problem, then re-submits as a different/correct function arguments, a malicious miner can change the order of the transactions, so the insecure one is the one that ends up taking effect, letting the attacker deploy the malicious proxy.
EIP-712 has stressed upon replay attacks which can be checked here
Tools Used
Manual Review
Recommendations
Add an incremented nonce to signed data.
+ mapping(address => uint256) private _nonces;
+ function _useNonce(address organizer) internal virtual returns (uint256) {
+ // For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
+ // decremented or reset. This guarantees that the nonce never overflows.
+ unchecked {
+ // It is important to do x++ and not ++x here.
+ return _nonces[organizer]++;
+ }
+ }
function deployProxyAndDistributeBySignature(
address organizer,
bytes32 contestId,
address implementation,
bytes calldata signature,
bytes calldata data
) public returns (address) {
- bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
+ bytes32 hash = keccak256(abi.encode(contestId, data, _useNonce(organizer)));
+ bytes32 digest = _hashTypedDataV4(hash);
if (ECDSA.recover(digest, signature) != organizer) revert ProxyFactory__InvalidSignature();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
address proxy = _deployProxy(organizer, contestId, implementation);
_distribute(proxy, data);
return proxy;
}
