Community's escrow allows for signature replay

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L509-L552

Vulnerability details

checkSignatureValidity() verification by signature do not utilize nonces and can be tricked by using owner / builder signatures from earlier calls. Namely, while checkSignatureValidity's approvedHashes based way can used only once as it deletes the corresponding array entry, the _signature based logic can be reused and itself contains no nonce, just validating that the signature for the constant message is from the desired actor. This validation will go through for all the subsequent calls, which provides a way to bypass the verification by supplying previously recorded signature for the same hash used in a different operation.

Signatory replay is a low level issue that opens up a range of attack surfaces. Currently all the uses besides escrow() utilize nonces, so the impact consists of escrow() calls being repeated as there is no nonce in the data, reducing the debt up to zero.

Proof of Concept

escrow() doesn't has nonce in the _hash, so can be rerun with the same _data:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L509-L552

    function escrow(bytes calldata _data, bytes calldata _signature)
        external
        virtual
        override
        whenNotPaused
    {
        // Decode params from _data
        (
            uint256 _communityID,
            address _builder,
            address _lender,
            address _agent,
            address _project,
            uint256 _repayAmount,
            bytes memory _details
        ) = abi.decode(
                _data,
                (uint256, address, address, address, address, uint256, bytes)
            );

        // Compute hash from bytes
        bytes32 _hash = keccak256(_data);

        // Local instance of variable. For saving gas.
        IProject _projectInstance = IProject(_project);

        // Revert if decoded builder is not decoded project's builder
        require(_builder == _projectInstance.builder(), "Community::!Builder");

        // Revert if decoded _communityID's owner is not decoded _lender
        require(
            _lender == _communities[_communityID].owner,
            "Community::!Owner"
        );

        // check signatures
        checkSignatureValidity(_lender, _hash, _signature, 0); // must be lender
        checkSignatureValidity(_builder, _hash, _signature, 1); // must be builder
        checkSignatureValidity(_agent, _hash, _signature, 2); // must be agent or escrow

        // Internal call to reduce debt
        _reduceDebt(_communityID, _project, _repayAmount, _details);
        emit DebtReducedByEscrow(_agent);
    }

checkSignatureValidity() allows two ways of verification, where approvedHashes based way can used only once, while _signature based approach can be reused repeatedly:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L871-L893

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal virtual {
        // Decode signer
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );

        // Revert if decoded signer does not match expected address
        // Or if hash is not approved by the expected address.
        require(
            _recoveredSignature == _address || approvedHashes[_address][_hash],
            "Community::invalid signature"
        );

        // Delete from approvedHash. So that signature cannot be reused.
        delete approvedHashes[_address][_hash];
    }

This holds as SignatureDecoder's recoverKey() can be run with the same signature, there is no nonce, so it can be run for the same messageHash:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/SignatureDecoder.sol#L20-L41

    function recoverKey(
        bytes32 messageHash,
        bytes memory messageSignatures,
        uint256 pos
    ) internal pure returns (address) {
        if (messageSignatures.length % 65 != 0) {
            return (address(0));
        }

        uint8 v;
        bytes32 r;
        bytes32 s;
        (v, r, s) = signatureSplit(messageSignatures, pos);

        // If the version is correct return the signer address
        if (v != 27 && v != 28) {
            return (address(0));
        } else {
            // solium-disable-next-line arg-overflow
            return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);
        }
    }

Recommended Mitigation Steps

To target the root source nonce has to be presented in all the signature verifications, invalidating all the subsequent uses.

This way consider ensuring that in all the instances checkSignatureValidity() is called for the hash that includes a specific nonce for the functionality. Whenever this doesn't hold the signature replay is possible with full consequences being reasonably hard to track.

[horsefacts]

Duplicate of #161