receive() function in Router allows locking of accidentally sent user’s BNB

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

The receive() function is executed on a call to the contract with empty calldata and is the function that is executed on plain BNB transfers via .send() or .transfer(). The presence of an empty receive() function will allow arbitrary BNB accidentally sent to this contract to get locked in the contract causing fund loss to users.

It is not clear why this is required given that the logic should only support receiving of BNB by the specific payable functions (add/swap assets) as it already does.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L39

payable functions:
https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L46

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L51

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L74

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L79

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L154

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L163

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L168

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L229

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove this function or document why it is required.

[verifyfirst]

Recieve function is needed for BNB transfers

[ghoul-sol]

Making this non-critical as it is a best practice.