BlazeSwapERC20 Manual Review Findings

BSE-01M: Insecure Elliptic Curve Recovery Mechanism

Type	Severity	Location
Language Specific		BlazeSwapERC20.sol:L105, L107

Description:

The ecrecover function is a low-level cryptographic function that should be utilized after appropriate sanitizations have been enforced on its arguments, namely on the s and v values. This is due to the inherent trait of the curve to be symmetrical on the x-axis and thus permitting signatures to be replayed with the same x value (r) but a different y value (s).

Impact:

Should the payload being verified by the signature rely on differentiation based on the s or v arguments, it will be possible to replay the signature for the same data validly and acquire authorization twice.

Example:

Copy
100function permit(
101    address owner,
102    address spender,
103    uint256 value,
104    uint256 deadline,
105    uint8 v,
106    bytes32 r,
107    bytes32 s
108) external {
109    require(deadline >= block.timestamp, 'BlazeSwap: EXPIRED');
110    bytes32 digest = keccak256(
111        abi.encodePacked(
112            '\x19\x01',
113            DOMAIN_SEPARATOR,
114            keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline))
115        )
116    );
117    address recoveredAddress = ecrecover(digest, v, r, s);
118    require(recoveredAddress != address(0) && recoveredAddress == owner, 'BlazeSwap: INVALID_SIGNATURE');
119    _approve(owner, spender, value);
120}

Recommendation:

We advise them to be sanitized by ensuring that v is equal to either 27 or 28 (v ∈ {27, 28}) and to ensure that s is existent in the lower half order of the elliptic curve (0 < s < secp256k1n ÷ 2 + 1) by ensuring it is less than 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1. A reference implementation of those checks can be observed in the ECDSA library of OpenZeppelin and the rationale behind those restrictions exists within Appendix F of the Yellow Paper.

Alleviation:

The BlazeSwap team assessed this finding and deemed it as a non-issue given that the payloads signed are using a nonce system and thus a replay attack would not be feasible even with a malleable signature. We would like to note that it is still best practice to disallow malleable signatures and we will consider this exhibit acknowledged while downgrading the severity level to informational.

BSE-02M: Cross-Chain Replay Attack

Type	Severity	Location
Language Specific		BlazeSwapERC20.sol:L110-L116

Description:

The permit mechanism of the BlazeSwapERC20 token is susceptible to cross-chain replay attacks as it utilizes a DOMAIN_SEPARATOR that is initialized solely during the construction of the contract.

Impact:

Although rare, all permits of the contract will be replay-able on a forked chain in the current implementation which can lead to loss of funds in the forked environment and vice-versa.

Example:

Copy
21constructor() {
22    uint256 chainId = block.chainid;
23    DOMAIN_SEPARATOR = keccak256(
24        abi.encode(
25            keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
26            keccak256(bytes(name)),
27            keccak256(bytes('1')),
28            chainId,
29            address(this)
30        )
31    );
32}

Recommendation:

We advise a similar approach to OpenZeppelin's draft-ERC20Permit to be utilized instead, calculating the domain separator of EIP-712 on a need-to basis as it relies on the chainId argument which can vary in the execution environment of the contract.

Alleviation:

The codebase has been updated implementing a caching system for the domain separator and re-calculating it in case the block.chainid has changed since the initial generation of the separator, ensuring that the contract is safe against cross-chain replay attacks and alleviating this exhibit in full.

BSE-03M: Inexistent EIP-20 Approval Event

Type	Severity	Location
Standard Conformity		BlazeSwapERC20.sol:L90-L92

Description:

The transferFrom function does not properly emit the EIP-20 Approval event while it adjusts the allowance entry.

Impact:

Currently, the contract is non-conformant with the EIP-20 standard in relation to the transferFrom function and may cause integration issues with off-chain block explorers.

Example:

Copy
85function transferFrom(
86    address from,
87    address to,
88    uint256 value
89) external returns (bool) {
90    if (allowance[from][msg.sender] != type(uint256).max) {
91        allowance[from][msg.sender] = allowance[from][msg.sender] - value;
92    }
93    _transfer(from, to, value);
94    return true;
95}

Recommendation:

We advise the Approval event to be emitted properly ensuring off-chain blockchain explorers can properly track allowance changes for transferFrom operations.

Alleviation:

The BlazeSwap team stated that the approval event emitted during a transferFrom operation is not mandated by the EIP-20 standard and as such this exhibit can be safely ignored. After investigating on our end, we find this statement is correct and thus nullify this exhibit.