Closed code423n4 closed 2 years ago
Duplicate of #66
We are aware that the contract allows users to use latent funds, although we disagree on it being an issue as no funds (ERC20 or native) should ever lay in the contract. To make sure that no value is ever kept by the diamond, we now provide refunds for outstanding user value (after bridges/swaps).
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-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L22-L30
https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L29-L58
The
Swapper
allow arbitrary_swapData
(0x style), this makes it possible for a attacker to steal the funds in the contract.Based on the context, we beleive 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
.PoC
Given:
The attacker can submit a
swapTokensGeneric()
withUSDT
asreceivingAssetId
with the followingSwapData[]
:As a result, the attacker will receive ~100 USDT with 0 USDC paid.
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.