code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-rolla/blob/a06418c9cc847395f3699bdf684a9ac066651ed7/quant-protocol/contracts/timelock/TimelockController.sol#L100

Vulnerability details

If a contract has a payable function, the contract must create a way to transfer funds out or else the funds will be locked in the contract forever. If a contract is payable someone will invariably try to pay it at some point in the course of its life.

Impact

Any ether sent to TimelockController without accompanying calldata is locked in the contract forever. The sender has no way to get the funds back out and is given nothing in return. See this similar instance which was found to be a medium issue.

Proof of Concept

    receive() external payable {}

https://github.com/code-423n4/2022-03-rolla/blob/a06418c9cc847395f3699bdf684a9ac066651ed7/quant-protocol/contracts/timelock/TimelockController.sol#L100

Tools Used

Code inspection

Recommended Mitigation Steps

Remove the receive() function

quantizations commented 2 years ago

Users should be cautious when sending money anywhere. Sending money to any account or contract you don't have control will always cause issues and it a case of user error.

alcueca commented 2 years ago

The referenced issue for NestedFinance was valid because the payable contract was expected to receive WETH, so sending ETH by error was a likely event.

Users have no reason to interact with TimelockController, and much less to send any assets to it with an expectation of getting anything back.

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