search

code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

Whitelisting should be by function signature and not only on contract address #108

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/Swapper.sol#L16 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibStorage.sol#L5

Vulnerability details

Impact

Currently, whitelisting for the swapper facet is build with an address whitelisting feature. However, for some use cases, it would be preferable to add an other layer of protection with a function signature whitelisting.

Proof of Concept

For example, you may want to add the possibility to wrap / unwrap ETH. But then whitelisting the contract (https://etherscan.io/address/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2#code) would enable anyone to call the approve function using the swapper and to withdraw any wETH from the LiFiDiamond contract.

Such behavior would also happen for any pool that is also an ERC20 token.

Overall, I think for the stake of security this additional layer is required as it diminished the attack surface.

Recommended Mitigation Steps

Replace mapping(address => bool) dexWhitelist; with a double mapping mapping(address => mapping(bytes4 => bool) ) dexWhitelist;. Then parse the field callData of SwapData accordingly to check for whitelist.

maxklenk commented 2 years ago

We "disagree with severity" as we have only added dexes to our allowlist to avoid such attacks. At the same time we had planned such a function signature allowlist as specified in the docs for this audit: https://github.com/code-423n4/2022-03-lifinance#planned-improvements

gzeoneth commented 2 years ago

Downgrading to Low/QA.

gzeoneth commented 2 years ago

Consider with warden's QA Report #114