search

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

1 stars 1 forks source link

Funds approved to OperateProxy can be stolen through the Call ActionType #16

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-rolla/blob/main/quant-protocol/contracts/Controller.sol#L153-L156 https://github.com/code-423n4/2022-03-rolla/blob/main/quant-protocol/contracts/Controller.sol#L117-L122 https://github.com/code-423n4/2022-03-rolla/blob/main/quant-protocol/contracts/Controller.sol#L556-L558

Vulnerability details

Impact

According to the comment, somebody will or might approve tokens to the OperateProxy contract linked to the Controller. These funds can be stolen by an attacker using the ActionType.Call which allows them to call any arbitrary contract + function using the proxy.

A contract doesn't have to hold any funds directly to be vulnerable. Having funds approved to it is the same thing if an attacker is allowed to call arbitrary contracts + functions.

Proof of Concept

  1. Alice approves the operate proxy to spend X amount of WETH
  2. Bob calls Controller.operate() with the ActionType.Call and triggers WETH's transferFrom function with the following arguments: [address(Alice), address(Bob), X].

Tools Used

none

Recommended Mitigation Steps

The code doesn't say why someone would approve tokens to the proxy. Depending on that you might design the protocol to NOT expect any funds to be approved to the proxy.

Even better might be to scratch the whole ActionType.Call. I'm not really sure what it is needed for.

quantizations commented 2 years ago

This is not really a bug as it's not intended to be used that way. Marking as low severity and we can advise users not to do that in both the comments and the docs.

alcueca commented 2 years ago

There are no funds immediately at risk, for that the user needs to first approve the proxy to use the funds. One severity level down.

There is nothing in the code that might imply that approvals should be given to the proxy, and so the user needs to do something that has no reason for, another severity level down.

Merging with the QA report as a documentation issue.

alcueca commented 2 years ago

Score of 14 as a QA Report (including points from #18)

JeeberC4 commented 2 years ago

Including with QA Report #18