search

code-423n4 / 2021-06-realitycards-findings

3 stars 2 forks source link

Possible locked-ether (funds) Issue in RCOrderbook.sol #43

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

maplesyrup

Vulnerability details

Impact

2 - Medium Risk

Proof of Concept

When running the analyzer code, the following functions were found in RCOrderbook.sol to possibly lock funds due to it being a payable function with no withdraw function associated.

Contract locking ether found:

// contracts/RCOrderbook.sol // line(s) 15-876

Contract RCOrderbook

has payable functions:

// contracts/lib/NativeMetaTransaction.sol // line(s) 31-67

NativeMetaTransaction.executeMetaTransaction(address,bytes,bytes32,bytes32,uint8)

But does not have a function to withdraw the funds

According to Slither analyzer detector documentation (https://github.com/crytic/slither/wiki/Detector-Documentation#contracts-that-lock-ether)

Possible functions that receive funds with the payable attribute must have a withdraw function to secure that funds can be sent out from the function or remove payable attribute.

Although the function may not receive funds directly, there should be a withdraw function added to ensure that information needed from the function can be withdrawn safely or do not include payable attribute.

Console Output (Slither log):

INFO:Detectors: Contract locking ether found: Contract RCOrderbook (contracts/RCOrderbook.sol#15-876) has payable functions:

Tools Used

Solidity Compiler 0.8.4 Hardhat v2.3.3 Slither v0.8.0

Compiled, Tested, Deployed contracts on a local hardhat network.

Ran Slither-analyzer for further detecting and testing.

Recommended Mitigation Steps

(Worked best under python venv)

  1. Clone Project Repository
  2. Run Project against Hardhat network; compile and run default test on contracts.
  3. Installed slither analyzer: https://github.com/crytic/slither
  4. Ran [$ slither .] against RCOrderbook.sol and all contracts to verify results
Splidge commented 3 years ago

I initially confirmed this because we aren't using the native currency on Matic/Polygon. However I think this should be disputed mainly because this function is used to call other functions which might be payable, although I admit currently we don't have payable functions, we might add them in the future. This library is used across all our contracts, had we put a payable function in the Treasury for instance, would this be considered a flaw to have this same library imported into the Orderbook?

Splidge commented 3 years ago

Note that the duplicate issue #51 was submitted by the same user.

dmvt commented 3 years ago

Agree with the sponsor's explanation, but the issue exists regardless. Adding a way to retrieve locked funds would mitigate the issue.