search

code-423n4 / 2022-01-openleverage-findings

0 stars 0 forks source link

Eth sent to Timelock will be locked in current implementation #80

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

defsec

Vulnerability details

Impact

Eth sent to Timelock will be locked in current implementation. I came across this problem while playing around with the governance contract.

Proof of Concept

    await proxy.propose(
      [signers[3].address],
      [ethers.utils.parseEther("0.1")],
      [""],
      [ethers.BigNumber.from(0)],
      "Send funds to 3rd signer"
    );
await proxy.execute(2);  // this fails
await proxy.execute(2, {value: ethers.utils.parseEther("0.1")})  // this would work
0.1 eth will be sent out, but it is sent from the msg.sender not from the timelock contract.

Tools Used

Recommended Mitigation Steps

Consider implementing the following code.

    function execute(uint proposalId) external {
        require(state(proposalId) == ProposalState.Queued, "GovernorAlpha::execute: proposal can only be executed if it is queued");
        Proposal storage proposal = proposals[proposalId];
        proposal.executed = true;
        for (uint i = 0; i < proposal.targets.length; i++) {
            timelock.executeTransaction(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
        }
        emit ProposalExecuted(proposalId);
    }

 Reference

https://github.com/compound-finance/compound-protocol/pull/177/files

0xleastwood commented 2 years ago

I agree with this finding!