search

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

6 stars 4 forks source link

[WP-H8] Admin of the upgradeable proxy contract (the diamond contract) can rug users #161

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/LiFiDiamond.sol#L7-L7

Vulnerability details

Use of Upgradeable Proxy Contract Structure (The Diamond Structure) allows the logic of the contract to be arbitrarily changed.

This allows the proxy admin to perform malicious actions e.g., taking funds from users' wallets up to the allowance limit.

This action can be performed by the malicious/compromised proxy admin without any restriction.

Considering that the main purpose of this particular contract is for bridging tokens with multiple bridge operators, we believe it's not necessary for the diamond contract to hold users' allowances.

PoC

Given:

Rug Users' Allowances

  1. Alice approve() and swapAndStartBridgeTokensViaAnyswap() with 1e8 USDC;
  2. Bob approve() and swapAndStartBridgeTokensViaAnyswap() with 5e8 USDC;
  3. A malicious/compromised proxy admin can deploy a malicious facet to the diamond contract and stolen all the USDC in Alice and Bob's wallets;

Severity

A smart contract being structured as an upgradeable contract alone is not usually considered as a high severity risk. But given the severe impact (funds in users' wallets got stolen), we mark it as a High severity issue.

Recommendation

Consider using the non-upgradeable Router contract to hold user's allowances instead. Then call transferFrom to pull funds from users' wallets to the the diamond contract and prcess the business login with the diamond contract.

H3xept commented 2 years ago

The bridges/swaps ecosystem is continually changing. This comes at the cost of having some degree of centralization. We chose the Diamond standard to be able to constantly add new bridges and update the existing ones as they improve and update.

H3xept commented 2 years ago

Duplicate of #65