Omniscia Xcaliswap Audit
SwapPair Manual Review Findings
SwapPair Manual Review Findings
SPR-01M: Potentially Misleading Fee Amounts
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | SwapPair.sol:L164, L178 |
Description:
The Fees event is meant to emit the total amount of fees acquired for either the token0 or token1 asset, however, the Xcaliswap protocol imposes a custom 10% tariff that is redirected to a protocol fee collector and does not benefit the pair.
Example:
153// Accrue fees on token0154function _update0(uint amount) internal {155 address _feeTo = ISwapFactory(factory).feeCollector();156 uint256 _protocolFee = amount / 10; // 10% of the amount157 uint256 _feeIncrease = amount - _protocolFee; // Might leave tokens in this contract due to rounding but ok, reserves updated after this function158 _safeTransfer(token0, _feeTo, _protocolFee);159 _safeTransfer(token0, fees, _feeIncrease); // transfer the fees out to SwapFees160 uint256 _ratio = _feeIncrease * 1e18 / totalSupply; // 1e18 adjustment is removed during claim161 if (_ratio > 0) {162 index0 += _ratio;163 }164 emit Fees(msg.sender, amount, 0);165}166
167// Accrue fees on token1168function _update1(uint amount) internal {169 address _feeTo = ISwapFactory(factory).feeCollector();170 uint256 _protocolFee = amount / 10; // 10% of the amount171 uint256 _feeIncrease = amount - _protocolFee; // Might leave tokens in this contract due to rounding but ok, reserves updated after this function172 _safeTransfer(token1, _feeTo, _protocolFee); // Transfer protocol fee to _feeTo173 _safeTransfer(token1, fees, _feeIncrease); // transfer the fees out to SwapFees174 uint256 _ratio = _feeIncrease * 1e18 / totalSupply;175 if (_ratio > 0) {176 index1 += _ratio;177 }178 emit Fees(msg.sender, 0, amount);179}Recommendation:
We advise the Fees events to either introduce a new variable denoting the protocol fee or to instead emit the _feeIncrease value instead as otherwise it is misleading.
Alleviation:
The Xcaliswap team has fixed this issue by emitting the _feeIncrease value in the Fees events.
SPR-02M: Insecure Elliptic Curve Recovery Mechanism
| Type | Severity | Location |
|---|---|---|
| Language Specific |  | SwapPair.sol:L477, L495 |
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:
477function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external {478 require(deadline >= block.timestamp, 'SwapPair: EXPIRED');479 DOMAIN_SEPARATOR = keccak256(480 abi.encode(481 keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),482 keccak256(bytes(name)),483 keccak256('1'),484 block.chainid,485 address(this)486 )487 );488 bytes32 digest = keccak256(489 abi.encodePacked(490 '\x19\x01',491 DOMAIN_SEPARATOR,492 keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline))493 )494 );495 address recoveredAddress = ecrecover(digest, v, r, s);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:
While the Xcaliswap team has added require statements to validate the v and s values, the validation condition for s has been implemented incorrectly. The bytesX and unitary denominations (i.e. uintX) may differ in underlying binary representation, hence the s variable should be casted to uint256 before comparing it to 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1.