code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

RemoteOwner .execute should be payable #154

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/remote-owner/blob/285749ab51e98afc8ebb4e4049a4348d669a3e9d/src/RemoteOwner.sol#L63-L75

Vulnerability details

Impact

The execute function in the RemoteOwner is used by the owner to call another contract. This call can be made with an ether value as specified in the contract. BUt there is no real way to call another contract with Ether, this is because the function not payable and the contract and its inherited contract do not have any receive payable functions. This has a significant impact on operations that can be executed

Proof of Concept

function execute(address target, uint256 value, bytes calldata data) external returns (bytes memory) {
    // console2.log("EXECUTE");
    // console2.logBytes(data);
    _checkSender();
    (bool success, bytes memory returnData) = target.call{ value: value }(data);

No payable receive or fallback either https://github.com/GenerationSoftware/ERC5164/blob/main/src/abstract/ExecutorAware.sol https://github.com/GenerationSoftware/remote-owner/blob/285749ab51e98afc8ebb4e4049a4348d669a3e9d/src/RemoteOwner.sol

Tools Used

Manual review

Recommended Mitigation Steps

Make the execute function payable or/and a receive function so the contract can receive ether

Assessed type

Payable

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #66

raymondfam commented 1 year ago

The severity should be medium with no direct loss of funds.

c4-judge commented 1 year ago

HickupHH3 changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

HickupHH3 marked the issue as satisfactory

c4-judge commented 1 year ago

HickupHH3 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

HickupHH3 marked the issue as grade-c