Party DAO - Vagner's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L146-L148

Vulnerability details

Impact

The function executeProposal in ProposalExecutionEngine.sol that is called with delegatecall in PartyGovernance.sol does not have payable keyword, which means that anytime the msg.value of the call is greater than 0, the call will revert, making executing proposals impossible.

Proof of Concept

The process of executing a proposal starts in PartyGovernance.sol by calling execute after a proposal is ready or InProgress. As you can see, the function is payable since it expects that in some situations, msg.value is greater than 0 so it can be used later https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L706-L713 After multiple checks it goes into _executeProposal where it will delegatecall into ProposalExecutionEngine.sol, to the executeProposal function https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L884-L888 As you can see this function does not have the payable keyword https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L146-L148 which means that anytime a delegatecall is made into executeProposal with the msg.value of the whole call greater than 0, the call will just revert. We can see down the line one example where it is intended for msg.value to be greater than 0, the function _execute https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L194 can call into _executeArbitraryCalls https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L261-L263 which uses msg.value https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ArbitraryCallsProposal.sol#L74-L90, in this situation and many more, the call will revert 100% time, making executing proposal impossible. This thing can be checked easily by using 2 layers of simple contracts, that delegatecall to imitate the behavior of these 2 contract

contract TestContract {

function delegate(address _delegateTo, bytes calldata _calldata) external payable {

    (bool success, bytes memory returnData) = _delegateTo.delegatecall(_calldata);
    assembly {
        if iszero(success) {
            revert(add(returnData, 0x20), mload(returnData))
        }
    }
}
}

as the proxy contract of the Party.sol contract

contract SimpleContract{
  function notPayable() public returns (bool) {
  return true;    
 }
}

as the ProposalExecutionEngine.sol implementation contract. Even if the first function that delegatecalls is payable, if you try to do any calls towards SimpleContract where the msg.value is greater than 0, the call will just revert, but if you set the notPayable function with the payable keyword, the call will behave as expected.

Tools Used

Manual review

Recommended Mitigation Steps

Since you intend to use msg.value in executing the proposals, use payable keyword on the executeProposal function, so the call will not revert.

Assessed type

Payable