(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
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
GitHub
[WP-H6] Swapper can be used to steal all the funds from the contract · Issue #159 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L22-L30 Vulnerability details https://github.com/code-423...
(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.
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
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
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
GitHub
Any native assets in the LiFiDiamond can be took by anyone · Issue #16 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14-L21 Vulner...
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 6
Problem: No handling external call with msg.value
CBridgeFacet is not sending native asset to CBridge
Severity: Medium
Impact: _startBridge will always fail when sending native token.
Proof of Concept:
1. CBridgeFacet:_startBridge is not sending native asset will calling sendNative
ICBridge(bridge).sendNative(
2. sendNative will revert when msg.value != _amount
Mitigation:
ICBridge(bridge).sendNative{value: msg.value}(
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
Source, Code
Number: 6
Problem: No handling external call with msg.value
CBridgeFacet is not sending native asset to CBridge
Severity: Medium
Impact: _startBridge will always fail when sending native token.
Proof of Concept:
1. CBridgeFacet:_startBridge is not sending native asset will calling sendNative
ICBridge(bridge).sendNative(
2. sendNative will revert when msg.value != _amount
Mitigation:
ICBridge(bridge).sendNative{value: msg.value}(
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 7
Problem: Return while loop
DexManagerFacet: batchRemoveDex() removes first dex only
Severity: Medium
Impact:
The function intends to allow the removal of multiple dexes approved for swaps. However, the function will only remove the first DEX because return is used instead of break in the inner for loop.
if (s.dexs[j] == _dexs[i]) {
_removeDex(j);
// should be replaced with break;
return;
}
This error is likely to have gone unnoticed because no event is emitted when a DEX is added or removed.
Mitigation: Replace return with break.
if (s.dexs[j] == _dexs[i]) {
_removeDex(j);
break; (will end the current inner loop)
}
In addition, it is recommended to emit an event whenever a DEX is added or removed.
#CODE4ARENA #MEDIUM #REPORT #LOOP #LIFINANCE
Source, Code
Number: 7
Problem: Return while loop
DexManagerFacet: batchRemoveDex() removes first dex only
Severity: Medium
Impact:
The function intends to allow the removal of multiple dexes approved for swaps. However, the function will only remove the first DEX because return is used instead of break in the inner for loop.
if (s.dexs[j] == _dexs[i]) {
_removeDex(j);
// should be replaced with break;
return;
}
This error is likely to have gone unnoticed because no event is emitted when a DEX is added or removed.
Mitigation: Replace return with break.
if (s.dexs[j] == _dexs[i]) {
_removeDex(j);
break; (will end the current inner loop)
}
In addition, it is recommended to emit an event whenever a DEX is added or removed.
#CODE4ARENA #MEDIUM #REPORT #LOOP #LIFINANCE
GitHub
DexManagerFacet: batchRemoveDex() removes first dex only · Issue #34 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L71-L73 Vulnerability details Details & Impact The function intends to allow the remov...
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 8
Problem: Loss the msg.value
ERC20 bridging functions do not revert on non-zero msg.value
Severity: Medium
Impact:
Any native funds mistakenly sent along with plain ERC20 bridging calls will be lost. AnyswapFacet, CBridgeFacet, HopFacet and NXTPFacet have this issue.
For instance, swapping function might use native tokens, but the functions whose purpose is bridging solely have no use of native funds, so any mistakenly sent native funds to be frozen on the contract balance.
Placing the severity to be medium as in combination with other issues there is a possibility for user funds to be frozen for an extended period of time (if WithdrawFacet's issue plays out) or even lost (if LibSwap's swap native tokens one also be triggered).
In other words the vulnerability is also a wider attack surface enabler as it can bring in the user funds to the contract balance.
Medium despite the fund loss possibility as the native funds in question here are mistakenly sent only, so the probability is lower compared to direct leakage issues.
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
Source, Code
Number: 8
Problem: Loss the msg.value
ERC20 bridging functions do not revert on non-zero msg.value
Severity: Medium
Impact:
Any native funds mistakenly sent along with plain ERC20 bridging calls will be lost. AnyswapFacet, CBridgeFacet, HopFacet and NXTPFacet have this issue.
For instance, swapping function might use native tokens, but the functions whose purpose is bridging solely have no use of native funds, so any mistakenly sent native funds to be frozen on the contract balance.
Placing the severity to be medium as in combination with other issues there is a possibility for user funds to be frozen for an extended period of time (if WithdrawFacet's issue plays out) or even lost (if LibSwap's swap native tokens one also be triggered).
In other words the vulnerability is also a wider attack surface enabler as it can bring in the user funds to the contract balance.
Medium despite the fund loss possibility as the native funds in question here are mistakenly sent only, so the probability is lower compared to direct leakage issues.
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
GitHub
ERC20 bridging functions do not revert on non-zero msg.value · Issue #53 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L38-L48 Vulnerability details Impact Any native funds mistakenly sent along with plain ERC20 brid...
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 9
Problem: Overpaying the msg.value
Caller can lose ETH using the CBridgeFacet
Severity: Medium
Impact: A user wanting to bridge ETH via CBridge could lose some amount of ETH.
Proof of concept:
The function startBridgeTokenViaCBridge checks the amount of ETH transfered with msg.value >= _cBridgeData.amount in case the token address is zero.
If a user accidentally sends more ETH than _cBridgeData.amount, that ETH would be held unaccounted for in the contract and be lost for the user.
Mitigation:
Refactor the check to msg.value == _cBridgeData.amount.
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
Source, Code
Number: 9
Problem: Overpaying the msg.value
Caller can lose ETH using the CBridgeFacet
Severity: Medium
Impact: A user wanting to bridge ETH via CBridge could lose some amount of ETH.
Proof of concept:
The function startBridgeTokenViaCBridge checks the amount of ETH transfered with msg.value >= _cBridgeData.amount in case the token address is zero.
If a user accidentally sends more ETH than _cBridgeData.amount, that ETH would be held unaccounted for in the contract and be lost for the user.
Mitigation:
Refactor the check to msg.value == _cBridgeData.amount.
#CODE4ARENA #MEDIUM #REPORT #MSGVALUE #LIFINANCE
GitHub
Caller can lose ETH using the `CBridgeFacet` · Issue #96 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L68 Vulnerability details Impact A user wanting to bridge ETH via CBridge could lose some amount ...
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 10
Problem: Success even if contract no exist
Failed transfer with low level call won't revert
Severity: Medium
Impact:
swap is used throughout the code via _executeSwaps in Swapper.sol. According to Solidity Docs the call may return true even if it was a failure. This may result in user funds lost because funds were transferred into this contract in preparation for the swap. The swap fails but doesn't revert. There is a way this can happen through GenericSwapFacet.sol due to a missing require that is present in the other facets which is a separate issue but gives this issue more relevance.
Proof of concept:
1. Alice uses Generic swap with 100 DAI
2. Alice's 100 DAI are sent to the Swapper.sol contract
3. The call on swap _swapData.callTo.call{ value: msg.value }(_swapData.callData); fails but returns success due to nonexisting contract
4. postSwapBalance = 0
5. Alice receives nothing in return
Mitigation:
Check for contract existence
#CODE4ARENA #MEDIUM #REPORT #CALL #LIFINANCE
Source, Code
Number: 10
Problem: Success even if contract no exist
Failed transfer with low level call won't revert
Severity: Medium
Impact:
swap is used throughout the code via _executeSwaps in Swapper.sol. According to Solidity Docs the call may return true even if it was a failure. This may result in user funds lost because funds were transferred into this contract in preparation for the swap. The swap fails but doesn't revert. There is a way this can happen through GenericSwapFacet.sol due to a missing require that is present in the other facets which is a separate issue but gives this issue more relevance.
Proof of concept:
1. Alice uses Generic swap with 100 DAI
2. Alice's 100 DAI are sent to the Swapper.sol contract
3. The call on swap _swapData.callTo.call{ value: msg.value }(_swapData.callData); fails but returns success due to nonexisting contract
4. postSwapBalance = 0
5. Alice receives nothing in return
Mitigation:
Check for contract existence
#CODE4ARENA #MEDIUM #REPORT #CALL #LIFINANCE
GitHub
Failed transfer with low level call won't revert · Issue #101 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42-L46 Vulnerability details Impact swap is used throughout the code via _executeSwaps in Swapper....
(code-423n4/2022-03-lifinance-findings)
Source, Code
Number: 11
Problem: Use call instead of transfer and send
Use of transfer() may lead to failures
Severity: Medium
Proof of Concept:
src/Facets/WithdrawFacet.sol
31: payable(sendTo).transfer(_amount);
When withdrawing native token, the withdraw is being handled with a payable.transfer() call.
This is unsafe as transfer has hard coded gas budget and can fail.
Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.
Mitigation:
Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.
#CODE4ARENA #MEDIUM #REPORT #GAS #LIFINANCE
Source, Code
Number: 11
Problem: Use call instead of transfer and send
Use of transfer() may lead to failures
Severity: Medium
Proof of Concept:
src/Facets/WithdrawFacet.sol
31: payable(sendTo).transfer(_amount);
When withdrawing native token, the withdraw is being handled with a payable.transfer() call.
This is unsafe as transfer has hard coded gas budget and can fail.
Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.
Mitigation:
Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.
#CODE4ARENA #MEDIUM #REPORT #GAS #LIFINANCE
GitHub
Use of transfer() may lead to failures · Issue #15 · code-423n4/2022-03-lifinance-findings
Lines of code https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L31 Vulnerability details Proof of Concept src/Facets/WithdrawFacet.sol 31: payable(sendTo).tran...
(code-423n4/2022-04-abranft-findings)
Source, Code
Number: 12
Problem: Custom ERC721 can Reentrance
[WP-H8] Special ERC721 compatible implementation may allow an attacker to requestLoan without transferring in the NFT collateral
Code: https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L267-L297
Severity: Medium
NFT is a fragmented standard, for certain non-standard ERC721 implementations, they may have built-in hooks that can be used to re-enter the contract. Just like ERC777 to ERC20.
For example, if the collateral NFT got a pre-transfer hook to the receiver of the transfer, then it can be used to re-enter the contract and requestLoan without depositing the collateral.
Proof of Concept:
The malicious borrower can:
-requestLoan() with a collateral tokenId: 123;
-removeCollateral() for tokenId: 123:
(in the pre-transfer hook of collateral.transferFrom(address(this), to, tokenId);, re-enter requestLoan() with tokenId: 123 and skim: true)
-continue with transfer and the NFT is now back to the borrower.
As a result, the malicious borrower has effectively created a loan without depositing the collateral NFT.
Mitigation:
Consider adding require(collateral.ownerOf(tokenId) == address(this)); in _lend() to make sure the collateral tokenId is owned by the contract.
Attack:
RequestLoan()
RemoveCollateral (beforeTransfer -> requestLoan()) -> We got our collateral back, but still have the ability to lend
Lend() -> Should check whether the contract has the collateral, the original contract didn't have that check
#CODE4ARENA #MEDIUM #REPORT #ERC721 #REENTRANCY #ABRANFT
Source, Code
Number: 12
Problem: Custom ERC721 can Reentrance
[WP-H8] Special ERC721 compatible implementation may allow an attacker to requestLoan without transferring in the NFT collateral
Code: https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L267-L297
Severity: Medium
NFT is a fragmented standard, for certain non-standard ERC721 implementations, they may have built-in hooks that can be used to re-enter the contract. Just like ERC777 to ERC20.
For example, if the collateral NFT got a pre-transfer hook to the receiver of the transfer, then it can be used to re-enter the contract and requestLoan without depositing the collateral.
Proof of Concept:
The malicious borrower can:
-requestLoan() with a collateral tokenId: 123;
-removeCollateral() for tokenId: 123:
(in the pre-transfer hook of collateral.transferFrom(address(this), to, tokenId);, re-enter requestLoan() with tokenId: 123 and skim: true)
-continue with transfer and the NFT is now back to the borrower.
As a result, the malicious borrower has effectively created a loan without depositing the collateral NFT.
Mitigation:
Consider adding require(collateral.ownerOf(tokenId) == address(this)); in _lend() to make sure the collateral tokenId is owned by the contract.
Attack:
RequestLoan()
RemoveCollateral (beforeTransfer -> requestLoan()) -> We got our collateral back, but still have the ability to lend
Lend() -> Should check whether the contract has the collateral, the original contract didn't have that check
#CODE4ARENA #MEDIUM #REPORT #ERC721 #REENTRANCY #ABRANFT
(code-423n4/2022-01-yield-findings)
Source
Number: 13
Problem: Oracle data feed is insufficiently validated.
Severity: Medium
Impact: Price can be stale(out-of-date) and can lead to wrong quoteAmount return value
Proof of concept:
Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.
Price can be stale and can lead to wrong quoteAmount return value
#CODE4ARENA #MEDIUM #REPORT #ORACLE #YIELD_CONVEX
Source
Number: 13
Problem: Oracle data feed is insufficiently validated.
Severity: Medium
Impact: Price can be stale(out-of-date) and can lead to wrong quoteAmount return value
Proof of concept:
Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.
Price can be stale and can lead to wrong quoteAmount return value
#CODE4ARENA #MEDIUM #REPORT #ORACLE #YIELD_CONVEX