makerdao / dss-deploy

Set of smart contracts and bash scripts to deploy Multi collateral DAI
GNU Affero General Public License v3.0
68 stars 39 forks source link

DISCUSSION: Remove ETHJoin in favor of WEth #8

Closed MrChico closed 5 years ago

MrChico commented 5 years ago

Deploy scripts should be edited as well before this gets merged, but want to open this for discussions.

gbalabasquer commented 5 years ago

This was something we had pending to discuss, nice you brought it back. I think the WETH option is more generic and we can still provide native ETH usage via the proxy (though with a bit more gas consumption). We could also provide both adapters for the same collateral, however that could complicate things at the moment of exit if the amount is higher than the balance of your desired choice.

rainbreak commented 5 years ago

Each collateral can have multiple adapters, so you can offer both weth and native ether as options.

MrChico commented 5 years ago

Each collateral can have multiple adapters, so you can offer both weth and native ether as options.

Sure, but each adapters would add attack surface, and could complicate the risk assessment for the collateral. My main reason for wanting to avoid ETHJoin is that to avoid adding blind calls. I don't think it provides much of a benefit over weth + GemJoin.

MrChico commented 5 years ago

It may seem like the security profile of ETHJoin is similar to that of weth, but one key difference is that makes ETHJoin a lot more dangerous is that it makes blind calls AND has direct authority to the vat. It is not safe independent of gas – consider an attacker contract with a fallback function that upon invocation DELEGATECALLS into vat.rely to add a backdoor.

gbalabasquer commented 5 years ago

It may seem like the security profile of ETHJoin is similar to that of weth, but one key difference is that makes ETHJoin a lot more dangerous is that it makes blind calls AND has direct authority to the vat. It is not safe independent of gas – consider an attacker contract with a fallback function that upon invocation DELEGATECALLS into vat.rely to add a backdoor.

I get the context of the case and I'm actually liking more the idea to have a GemJoin and let the proxies to care the wrapping, but not getting the example you gave about delegatecalls to add the backdoor. How would that be possible if what the EthJoin is a plain call via transfer?

MrChico commented 5 years ago

The transfer from ethjoin could call an adversary contract A who would then delegatecall into vat, the msg.sender would then be Ethjoin

MrChico commented 5 years ago

Actually, it wouldnt be able to change the storage of the vat though, my bad

gbalabasquer commented 5 years ago

@MrChico I've created a new PR #14 in order to rebase master on your branch and do some changes in the deployment scripts (so it also uses WETH). This last thing is still not done, so will work on that and then I'll do the merge.