Cyfrin / 2023-08-sparkn

Other
11 stars 15 forks source link

Risk of Duplicating Execution in `deployProxyAndDistributeBySignature` Function #11

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Risk of Duplicating Execution in deployProxyAndDistributeBySignature Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L152-L167

Summary

The deployProxyAndDistributeBySignature function in the contract is susceptible to replay attacks due to the absence of a nonce or unique identifier in the signed message. This vulnerability could allow an attacker to reuse a valid signature to execute the function multiple times, even if the context has changed.

Vulnerability Details

The deployProxyAndDistributeBySignature function uses an ECDSA signature to authenticate the caller's address. However, it lacks protection against replay attacks because it does not include a nonce or unique identifier in the signed message. The following code snippet illustrates this issue:

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();
    // ...
}

The function generates a digest from the provided parameters and compares the recovered address with the organizer's address to validate the signature. However, due to the absence of a nonce or unique identifier, an attacker could capture a valid signature for a transaction and replay it later, potentially causing unintended consequences.

Impact

Exploiting this vulnerability could lead to an attacker repeatedly executing the deployProxyAndDistributeBySignature function, even if the original context or conditions have changed. This could result in multiple proxy deployments and prize distributions for a single contest, leading to misallocation of resources and disruptions to the intended contract behavior.

Tools Used

Manual

Recommendations

Include a nonce or unique identifier in the message that is signed. This ensures that each signature corresponds to a specific transaction and prevents replaying the same signature in different contexts.

PatrickAlphaC commented 1 year ago

This isn't an issue right now (I don't think) i'd need to see a PoC...

Moving to low to check with sponsor. I think the impact is none. I'm not sure a user could do a replay attack. They could front-run the tx I guess but why would they want to do that?