Several standards that can be used by a client to sign data using an Ethereum key:
-eth_sign:
-personal_sign: The method prefixed any signed data with "\x19Ethereum Signed Message:\n" which meant if one was to sign transaction data, the added prefix string would make it an invalid transaction.
-EIP-712: more complex use cases, this is done by signing hashes of all required configuration data (address, chain id, version, data types) + the actual data itself.
***Which signing standards should I use?'. From a contract's perspective use the latest EIP-712 standard! eth_sign is not safe and personal_sign is mostly useful for implementing user sign in features. In your contracts stick to EIP-712.

Now let's see how to implement EIP-712 in Solidity. The rough idea is
-compute a domain hash which captures the configuration data of contract address and chainId
-compute typed data hash
-combine both hashes and use it inside ecrecover

***Security issues with ecrecover + solution
1) In some cases ecrecover can return a random address instead of 0 for an invalid signature. This is prevented above by the owner address inside the typed data.
2) Signature are malleable, meaning you might be able to create a second also valid signature for the same data. In our case we are not using the signature data itself (which one may do as an id for example).
3) An attacker can construct a hash and signature that look valid if the hash is not computed within the contract itself.
4) In practice I would recommend once again to use the Openzeppelin contracts. Their ECDSA implementation solves all three problems and they have an EIP-712 implementation (still a draft but usable in my opinion). Not only is this easier to use, but they also have further improvements:
—caching mechanism for eip712DomainHash, so it's only calculated whenever chainId changes (so usually just once)
—additional security checks for the signature as mentioned above
—-ability to send signature as string


SIGNATURE REPLAY ATTACK:
1) Can be performed on the same contract (keeping track of what transactions have been executed inside the contract and nonces)
2) Same code, different address (Adding the address of the contract inside the signature, nounce)
3) A contract created with CREATE2 with self-destruct function inside (Deployed at the same address) Can't be prevented


https://solidity-by-example.org/hacks/signature-replay/

#EDUCATION

1) bytes32 typeHash = keccak256(
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
);
bytes32 hashedName = keccak256(bytes(name));
bytes32 hashedVersion = keccak256(bytes(version));

2) domainSeparator = keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, address(this)));

3) bytes32 private constant _PERMIT_TYPEHASH =
keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

4) bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

The necessary functionality
5) keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); <- How the signature is being constructed, the domainSeparator

Original code:
bytes32 digest = keccak256(abi.encodePacked("\x19\x01", keccak256(abi.encode(PERMIT_TYPEHASH, holder, spender, nonce, expiry, allowed))):

RESULT:
1) Digest has missing domainSeparator
2) The value was also missed -> value MaX (dangerous) or incorrect signature
3) pass digest to mapping()
4) block.timestamp check
5) get nonce inside of the function, delete the parameter nonce
#AUDIT #DONE

—Learn from Reading Audit Reports (Sturdy Report)
https://www.youtube.com/watch?v=9-Ye1HTvQIU

—My First Bounty Award
https://www.youtube.com/watch?v=vNB1t2Rpbtw

—Unhacked CTF - Reaper
https://www.youtube.com/watch?v=1xnMxdfYOG0

—Over 10k Achieved in Web3 Bounties
https://www.youtube.com/watch?v=P7omSW0S8nY

—My Study Methodology
https://www.youtube.com/watch?v=95Jj3yxVxNI

—My CV - Getting a JOB as a Smart Contract Auditor
https://www.youtube.com/watch?v=OFyB6BdTC8g

—My First Month as a Smart Contract Auditor
https://www.youtube.com/watch?v=AtTb_BpFZkM

—Code4rena Automation, Breaking Through Plateaus and Auditing Advice for Beginners/Intermediates
https://www.youtube.com/watch?v=gd5z2AKbvHk

—The ideal smart contract audit report
https://www.youtube.com/watch?v=cpyQw5mwE_0&t=331s

(code-423n4/2022-03-lifinance-findings)
Source , Code

Number: 1
Problem: approveERC20() uses unlimited approval
Severity: Medium

Impact:
approveERC20() is using unlimited approval.
Although unlimited approval is used by various DeFi platforms to minimize transaction fees and improve user experience, it introduces security risks as well.

Mitigation steps: approveERC20() -> approve( only required amount) (approve-spend pattern), or rather using increaseERC20Allowance()
/ decreaseERC20Allowance().
-to offer a choice: approve only what they need to spend at the time, or a much larger amount(future) (Zapper.fi, Curve.fi)
- EIP2612

Proof of Concept:
-Bancor, early 2020 (function calling tranferFrom was made public)
-Fucurombo (February 2021)
-Unicats (rugpull, 2020)
-Degen Money(frontend, mindless signing)
-Primitive Finance
-DeFi Safer

#CODE4ARENA #MEDIUM #REPORT #ERC20 #LIFINANCE

(code-423n4/2022-03-lifinance-findings)
Source, Code

Number: 2
Problem: approveMax for malicious routers leads to loss of funds.
AnyswapFacet can be exploited to approve arbitrary tokens.
Severity: Medium

Impact:
In AnyswapFacet.sol we parse arbitrary data in _anyswapData allowing an attacker to drain funds (ERC20 or native tokens) from the LiFi contract.

Functions effected:
-AnyswapFacet.startBridgeTokensViaAnyswap()
-AnyswapFacet.swapAndStartBridgeTokensViaAnyswap()
(multiple call of underlying() that can replace the the token at some point)

Proof of Concept
This attack works -in AnyswapFacet.startBridgeTokensViaAnyswap() by having a malicious _anyswapData.token which may change the value return in IAnyswapToken(_anyswapData.token).underlying() -> allows for transferring these malicious ERC20 tokens to pass the required balance checks.
-execute the following which will either approve or transfer funds to _anyswapData.router for a different underlyingtoken to the one which supplied the funds to LiFi.
-Since _anyswapData.router is an address in the attackers control they either are transferred native tokens or they have an allowance of ERC20 tokens that they can spend arbitrarily.

Mitigation steps:
Consider whitelisting both Anyswap tokens and Anyswap routers (using two distinct whitelists) restricting the attackers ability to use malicious contracts for this attack.
Consider also only calling IAnyswapToken(_anyswapData.token).underlying() once and passing this value to _startBridge().

There can be fund leftover in the contract under normal operation, for example this tx. In fact, ~$300 worth of token is left in the LI.Fi smart contract on ETH mainnet 0x5a9fd7c39a6c488e715437d7b1f3c823d5596ed1 as of block 14597316. I don't think this is High Risk because the max amount lost is no more than allowed slippage, which can be loss to MEV too.

#CODE4ARENA #MEDIUM #REPORT #ERC20 #LIFINANCE

(code-423n4/2022-03-lifinance-findings)
Source, Code

Number: 3
Problem: Can get remains token in this contract
Swapper can be used to steal all the funds from the contract
Severity: Medium

Vulnerability details:
The Swapper allow arbitrary _swapData( _lifiData ) (0x style), this makes it possible for a attacker to steal the funds in the contract.
Based on the context, we believe it's possible that the contract can hold funds.
The funds can be the refunds of failed orders, or fee rebates from bridging or dex aggregators, etc.
See also the permissioned WithdrawFacet.


Proof of Concept:
Given:
There are 100 USDC tokens in the contract.
The attacker can submit a swapTokensGeneric() with USDT as receivingAssetId with the following SwapData[]:

100 USDC to USDT;
As a result, the attacker will receive ~100 USDT with 0 USDC paid.

Mitigation/recommendation
Given that Swapper is a standlone module that can be and should be deployed as a standalone contract, we suggest spin it off from the diamond so that it can no longer access the funds in the diamond contract.

#CODE4ARENA #MEDIUM #REPORT #INVALIDCHECK #LIFINANCE

(code-423n4/2022-03-lifinance-findings)
Source, Code

Number: 4
Problem: Token will remain in the contract
LibSwap: Excess funds from swaps are not returned
Severity: Medium

Impact:
It is probable for _swapData.fromAmount to be greater than the actual amount used (eg. when swapping for an exact output, or when performing another swap after swapping with an exact input). However, these funds aren’t returned back to the user and are left in the lifi contract.

Proof of Concept:
https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/AnyswapFacet.test.ts#L153-L194
The test referenced above swaps for MATIC for 1000 USDT exactly. Logging the matic amounts before and after the swap and bridge call, one will find 18.01 MATIC is unused and left in the contract when it should be returned to the user.

Mitigation

this issue would not allow to access other users funds and only happens if the user passes these kind of swaps himself. The multi-swap feature is mainly used to allow swapping multiple different tokens into one, which is then fully swapped.

#CODE4ARENA #MEDIUM #REPORT #TRANFER #LIFINANCE

(code-423n4/2022-03-lifinance-findings)
Source, LibSwap, Swapper

Number: 5
Problem: msg.value inside the loop
Any native assets in the LiFiDiamond can be took by anyone
Severity: Medium

Proof of Concept:
-Never use msg.value inside a loop.
1) LibSwap.swap use msg.value when swapping native assets.

(bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);

2) Swapper call LibSwap.swap inside a loop.

for (uint8 i; i < _swapData.length; i++) {
require(
ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,
"Contract call not allowed!"
);

LibSwap.swap(_lifiData.transactionId, _swapData[i]);
}

3) Take GenericSwapFacet as a example
Attacker can call swapTokensGeneric with 1 ether with _swapData.length == 10. This will swap 10 ether to SwapData.callTo.

function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

// Swap
_executeSwaps(_lifiData, _swapData);

uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);

Mitigation:
The amount of native assets to be sent should be specified in _swapData. Check that total amount <= msg.value.

#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE