Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`ProxyFactory#deployProxyAndDistributeBySignature` is prone to signature replay attacks

madalad

Summary

With certain preconditions, it is possible to perform a replay attack through deployProxyAndDistributeBySignature. In this case, a malicious user could call deployProxyAndDistributeBySignature without being the organizer, and execute an unintended payout distribution.

Vulnerability Details

In deployProxyAndDistributeBySignature, the signature depends on the contestId and data parameters, but not on the implementation. Whereas in setContest, the unique contest identifier salt depends on contestId, organizer and implementation.

File: src\ProxyFactory.sol
​
106: function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
107: public
108: onlyOwner
109: {
110: if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
111: if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
112: revert ProxyFactory__CloseTimeNotInRange();
113: }
114: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
115: if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
116: saltToCloseTime[salt] = closeTime;
117: emit SetContest(organizer, contestId, closeTime, implementation);
118: }
​
...
​
152: function deployProxyAndDistributeBySignature(
153: address organizer,
154: bytes32 contestId,
155: address implementation,
156: bytes calldata signature,
157: bytes calldata data
158: ) public returns (address) {
159: bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
160: if (ECDSA.recover(digest, signature) != organizer) revert ProxyFactory__InvalidSignature(); // @audit signature does not depend on implementation
161: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
162: if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
163: if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
164: address proxy = _deployProxy(organizer, contestId, implementation);
165: _distribute(proxy, data);
166: return proxy;
167: }

Consider the following scenario:

  • owner calls setContest where organizer is Alice, contestId is 5 and implementation is the address of the Distribution contract

  • the contest plays out as expected, and at the end Alice calls deployProxyAndDistributeBySignature to distribute the awards by passing winners and percentages arrays using the data parameter

  • Alice holds another contest, and the owner calls setContest again, using the same contestId (this is not prevented), but a different implementation address (perhaps a future version of Distribution). The calculated salt will be unique as implementation has changed, and so the check on line 115 will pass

  • After the contest period is over, Alice is able to distribute the rewards, but malicious user Bob calls deployProxyAndDistributeBySignature before Alice, using the signature from the first contest, and the same data parameter as the first contest

  • The signature will be the same, as organizer, contestId and data are the same, and so the call will execute and rewards will be distributed to the same winners and with the same percentages as the first contest, without Alice's permission

Bob may have had a significant payout in the first contest, and so would profit largely even if he was not a sponsor for the second contest. Even if the owner of ProxyFactory intends to use a different contestId for each contest, this is never validated within the smart contract.

Impact

A malicious user can perform a signature replay attack to distribute the tokens in a way that is not intended by the owner nor the organizer, potentially profiting.

Tools Used

Manual review

Recommendations

Alter the calculation of digest so that a signature is not reusable in the case where organzier and contestId remain the same but implementation changes.

function deployProxyAndDistributeBySignature( // @audit can be frontrun and implementation changed !!
address organizer,
bytes32 contestId,
address implementation,
bytes calldata signature,
bytes calldata data
) public returns (address) {
- bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
+ bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data, implementation)));
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;
}

Alternatively, enforce the use of unique contestId values for each contest:

​
+ mapping(bytes32 contesdId => bool hasBeenUsed) public contestIdUsed;
​
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
+ require(!contestIdUsed[contestId], "contestId already been used.");
if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
revert ProxyFactory__CloseTimeNotInRange();
}
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
saltToCloseTime[salt] = closeTime;
+ contestIdUsed[contestId] = true;
emit SetContest(organizer, contestId, closeTime, implementation);
}

Support

FAQs

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

Give us feedback!