Feat: introduce `Safebox` contracts #1

amusingaxl · 2022-10-25T14:45:43Z

A safebox for digital assets.

The owner can request funds to be sent to the recipient.
The custodian can deny the request within a predefined time period (24h).
After the timelock, anyone can execute the withdrawal.
The custodian cooperation is required whenever the owner wants to update the recipient.

dbkbali

Looks good to me - based on our discussions

README.md

gbalabasquer · 2022-11-09T13:57:41Z

src/Safebox.sol

+     * @param to The destination address.
+     * @param amount The amount to be transfered.
+     */
+    function _safeTransfer(address token, address to, uint256 amount) internal {


I'd reduce the verbosity of the code just using a simple transfer. I don't see Maker interacting with a token that doesn't revert and this contract is behind governance setting it up for a specific purpose. This is only useful for contracts that are open to any token.
At least I'd remove the internal function and just put the code in withdraw (same for deposit) as there is not reuse of it. But honestly I'd just keep it simple with the regular transfer.

Makes sense, removed.

Another thing I considered was to make token a constructor parameter instead of a parameter for withdraw.

As it is, the Safebox can be seen as multi-token, but there will probably never be a case for it to hold more than 1 asset.

rockyfour · 2022-11-09T13:57:45Z

src/Safebox.sol

+     * @param data The new value of the parameter.
+     */
+    function file(bytes32 what, address data) external auth {
+        if (what == "recipient") {


This should be blocked when VAT is not live.

rockyfour · 2022-11-09T14:02:03Z

src/Safebox.sol

+
+    /**
+     * @notice Approves the change in the recipient.
+     * @dev Reverts if `previousRecipient` has not been set or if `_recipient` does not match it.


previousRecipient => pendingRecipient ?

rockyfour · 2022-11-09T14:15:02Z

src/Safebox.sol

+ * @title A safebox for digital assets.
+ * @notice
+ * - The `owner` is in full control of how much and when it can send assets to `recipient`.
+ *   - If MakerDAO governance ever executes an Emergency Shutdown, anyone can send assets to `recipient`.       This prevents assets being stuck in this contract when the governance smart contract is no longer operational.


This line is really long

Visual glitch on my editor. Already fixed.

rockyfour · 2022-11-09T14:16:44Z

src/Safebox.sol

+     * @param token The token withdrawn.
+     * @param amount The amount withdrawn.
+     */
+    event Withdraw(address indexed token, uint256 amount);


This should probably emit the recipient as well.

Done.

Also added the sender to the Deposit event.

rockyfour · 2022-11-09T14:24:03Z

README.md

+There are 3 "roles" in this contract:
+
+1. `owner`: has full control of how much and when it can send assets to `recipient`.
+2. `custodian`: approves changes in the `recipient` address made by the `owner`.


mmm namewise - this entity is not really storing/custodying the tokens, it can only block changing the recipient.

It has to be named like this for legal reasons.

rockyfour · 2022-11-09T14:29:35Z

src/Safebox.sol

+    /// @notice Addresses with owner access on this contract. `wards[usr]`
+    mapping(address => uint256) public wards;
+    /// @notice Addresses with custodian access on this contract. `can[usr]`
+    mapping(address => uint256) public can;


If custodian is effectively immutable and can only be set in the constructor why is this mapping needed?

Yeah this is a good observation. Let's just make an immutable variable and remove the mapping.

Coinbase requested the ability to be able to rotate their keys, so this will be needed. I'm adding the functionality right now.

…ians

rockyfour · 2022-11-10T13:04:20Z

src/Safebox.sol

+    /**
+     * @notice Check if an address has `custodian` access on this contract.
+     * @param usr The user address.
+     * @return Whether `usr` is a ward or not.


usr => custodian

rockyfour · 2022-11-10T13:08:33Z

src/Safebox.sol

+     * @notice Removes a custodian from this contract.
+     * @param usr The user address.
+     */
+    function removeCustodian(address usr) external override onlyCustodian {


Note that as with wards behavior, also here if there is more than one custodian this will allow any custodian to remove any other.
So need to make sure this is wanted.

Yes, that's the expected feature.

rockyfour · 2022-11-10T13:11:53Z

src/Safebox.sol

+     * @param _recipient The new recipient being approved.
+     */
+    function approveChangeRecipient(address _recipient) external override {
+        require(custodians[msg.sender] == 1, "Safebox/not-custodian");


Should this be replaced with the onlyCustodian modifier?

rockyfour · 2022-11-10T13:15:51Z

src/Safebox.sol

+
+        vat = VatAbstract(_vat);
+        token = GemAbstract(_token);
+        recipient = _recipient;


Consider emitting a RecipientChange event (and maybe changing the event name to RecipientSet or something similar).

All other events have "verb-like" structure (i.e.: Rely, Deny, AddCustodian), I'm gonna use SetRecipient for this.

rockyfour · 2022-11-10T13:27:42Z

src/Safebox.sol

+    constructor(address _vat, address _token, address _owner, address _custodian, address _recipient) {
+        require(_recipient != address(0), "Safebox/invalid-recipient");
+
+        wards[_owner] = 1;


Note that common behavior for MakerDao contracts is to only rely msg.sender in the constructor.

Yeah, we are aware of that, however, since Coinbase will be handling the deployment, we want to avoid them having to deny themselves afterwards.

Just to clarify, we are gonna provide them the parameters for deployment and then verify the contract after it's deployed. If there is a mismatch, the verification will fail and we will know.

Also the variables are public, so they could be easily checked after deployment & verification.

rockyfour · 2022-11-10T13:52:23Z

src/Safebox.sol

+     * @notice Deposits tokens into this contract.
+     * @param amount The amount of tokens.
+     */
+    function deposit(uint256 amount) external override {


Do you plan to use this function for depositing from the swap/output conduit?
Seems like current code passes the funds by specifying the recipient of buyGem.

You are right, I'm gonna remove that.
The idea was to keep track of Deposit events, but we can do the same by tracking Push from the RwaSwapOutputConduit contract.

`deposit` will most likely never be used, so we are removing it.

The-Arbiter

Hey guys, here is my brief analysis of SafeBox. I have extensive experience with escrows so I am reviewing it from the perspective of an escrow in "Maker style" :)
Since I am reviewing this in isolation, context may be missing with regards to some comments, however I have made various points as documented below:

TLDR

'Dumb depositing' ERC20 poses risks. I will go into this in the Attack Vectors section at the end.
Everything is override and I really don't think it should be (also nothing is virtual in SafeBoxLike.sol)
Some of the names could be changed to be more like regular Daiwanese, especially the addCustodian stuff
The structure of the 2 step ownership makes checking for address(0) edge cases mandatory however allows for smart contract wallets / recipients. This is a contextual thing, I want to confirm that this is intended.
IsOwner and IsCustodian seem redundant unless I'm missing some context here.
Comments and NATSPEC could do with trimming down depending on how dss-ish you want the code to look; this resembles how I wrote my own code prior to adapting to the dss style so this is just a general comment.

Documentation

This looks good, just needs updates when it's all complete.
I would appreciate some notes on why address(0) is checked for (i.e. to allow pendingRecipient to work only if it's initialised)

SafeBox.sol

AGPL license OK
All interfaces used
Not immediately clear to me how the Abstracts are used with the external view return functions in SafeBoxLike.sol but this is probably my lack of knowledge (have never tried to just return an imported interface before). Knowledge gap here so someone else needs to fill this in and make sure it's OK.
VatAbstract and GemAbstract OK - do we really need to use dss interfaces for only 3 interfaces? Personally I would use VatLike and GemLike.
Mappings OK - fixed spacing
Recipient and pending recipient vars OK
Auth mapping OK
onlyCustodian code OK - modifier name seems a bit out of place since it sits next to auth - not a big issue
Constructor OK
- Rely owner OK
- Add custodian OK
  I don't like AddCustodian as a name when we have rely already used as a keyword here.
- Set recipient OK
- Set vat OK
- Set gem OK
  I would change the param order to match either when they're used (sequentially) or make it alphabetically ordered, but that's me.
withdraw has an attack vector explained below but the code is OK.
rely - why is this override?
deny - same issue
file is OK - I made it public and not override but this is optional as it varies between implementations
The address(0) check is needed to stop issues with pendingRecipient and this should be noted in documentation.
isOwner - why do we need this? Can't they just query wards[usr]? I don't see a need for this (I may be missing context?) but the code is OK.
isCustodian - same as isOwner
addCustodian OK but should be called relyCustodian
removeCustodian OK but should be called denyCustodian
approveChangeRecipient - so I have some notes on this:
Existing implementation seems OK as it is. The benefit of it is that it doesn't rely on a call from the recipient's address. The drawback is that we have to ensure that the recipient isn't address(0) for this to be possible.
OZ has a 2 step transfer method however that requires that the recipient calls the accept function which works for EOAs but not things like smart contract wallets. In the end this depends on the usecase and I may be missing the context here.
No deposit function is present anymore (the previous deposit function looked OK though)

Attack Vectors

Assume that I am a malicious member of governance and decide to steal the funds of the SafeBox. I can use CREATE2crunch or another similar tool to determine the salt for a contract address which resembles an ERC20 token's address, then deploy my own ERC20 there which looks like the real thing, however, my transfer function is malicious.
While a deposit function would only transfer in tokens with address token (meaning that deposit and withdraw keeps a 'closed loop' such that the deposited token is the one being withdrawn), depositing directly to the contract would allow this kind of attack to occur if there was someone malicious responsible for the SafeBox deployment.

The-Arbiter · 2022-11-10T16:15:26Z

src/Safebox.sol

+    GemAbstract public immutable override token;
+
+    /// @notice Addresses with owner access on this contract. `wards[usr]`
+    mapping(address => uint256) public override wards;


Spacing

Suggested change

mapping(address => uint256) public override wards;

mapping (address => uint256) public override wards;

This is handled by prettier-plugin-solidity.

The-Arbiter · 2022-11-10T16:15:34Z

src/Safebox.sol

+    /// @notice Addresses with owner access on this contract. `wards[usr]`
+    mapping(address => uint256) public override wards;
+    /// @notice Addresses with custodian access on this contract. `custodians[usr]`
+    mapping(address => uint256) public override custodians;


Spacing

Suggested change

mapping(address => uint256) public override custodians;

mapping (address => uint256) public override custodians;

The-Arbiter · 2022-11-10T16:20:58Z

src/Safebox.sol

+    /// @notice Reference to the new recipient when it is to be changed.
+    address public override pendingRecipient;
+
+    modifier auth() {


Not crazy on the layout of code but it's subjective...

Maybe we should do it like autoline? (https://github.com/makerdao/dss-auto-line/blob/master/src/DssAutoLine.sol) where the rely deny and auth are together in the code? Up to you

We're using CES layout for this code.

The-Arbiter · 2022-11-10T16:37:13Z

src/Safebox.sol

+     * @param _custodian The safebox custodian.
+     * @param _recipient The recipient for tokens in the safebox.
+     */
+    constructor(address _vat, address _token, address _owner, address _custodian, address _recipient) {


Any reason for the ordering of the params here? Doesn't seem to follow any specific logic - up to you here but I would order it either alphabetically or based on order of use within the constructor (owner custodian recipient vat token). Again up to you

We're trying to logically group them:

MCD core modules > General contracts > Roles for this contract.

The-Arbiter · 2022-11-10T16:37:51Z

src/Safebox.sol

+        emit Rely(_owner);
+
+        custodians[_custodian] = 1;
+        emit AddCustodian(_custodian);


Why not RelyCustodian to keep it in line with existing naming conventions?

We're refraining from using Daiwanese for functions that are exclusively accessible to Coinbase.

The-Arbiter · 2022-11-10T16:46:57Z

src/Safebox.sol

+     * @notice Grants `usr` owner access to this contract.
+     * @param usr The user address.
+     */
+    function rely(address usr) external override auth {


Suggested change

function rely(address usr) external override auth {

function rely(address usr) external auth {

override is here to ensure it's implementing the same method declared in the SafeboxLike interface.

The-Arbiter · 2022-11-10T16:47:07Z

src/Safebox.sol

+     * @notice Revokes `usr` owner access from this contract.
+     * @param usr The user address.
+     */
+    function deny(address usr) external override auth {


Suggested change

function deny(address usr) external override auth {

function deny(address usr) external auth {

The-Arbiter · 2022-11-10T16:57:19Z

src/Safebox.sol

+     * @param what The changed parameter name. `"recipient"`
+     * @param data The new value of the parameter.
+     */
+    function file(bytes32 what, address data) external override auth {


Make this public and no override?

Suggested change

function file(bytes32 what, address data) external override auth {

function file(bytes32 what, address data) public auth {

Why public instead of external?

The-Arbiter · 2022-11-10T17:27:59Z

README.md

+There are 2 possible ways of making a deposit into the `Safebox`:
+
+1. Use the `Safebox` address as the `to` address of an ERC-20 compatible `transfer` transaction.
+2. Call the `deposit`<sup>[1]</sup> method from the `Safebox` contract:


If this is removed we should remove this :)

rockyfour · 2022-11-10T17:58:06Z

@amusingaxl can you please point us to the code of the input conduit with the support for post shutdown collateral distribution (basically the intended recipient of SafeBox)?

amusingaxl · 2022-11-10T20:04:08Z

Thank you for the thorough review @The-Arbiter.

Just providing some context:

We are between a rock and a hard place in a sense that we want to use the regular Daiwanese where it's possible, however there was some push back on that from the Coinbase side.

This contract is going to be operated by both MakerDAO Governance and Coinbase in a collaborative manner, so our strategy is trying to segregate the interfaces that each party will interact with. That's why we kept things like wards, file, etc. for the MakerDAO-facing methods, while the part that will mostly be accessed by Coinbase have "more common" names.

Contrary to most MCD contract out there, a ward is not able to add or remove "custodian" privileges, only another custodian can do so. That was explicitly requested by Coinbase, because they were not comfortable with the idea of MakerDAO Governance being freely able to replace them.

Now answering to some of your comments:

Everything is override and I really don't think it should be (also nothing is virtual in SafeBoxLike.sol)

In previous Solidity version you were required to override functions declared in interfaces, looks like I forgot this behavior changed in Solidity 0.8.8.

Some of the names could be changed to be more like regular Daiwanese, especially the addCustodian stuff

As explained above, we are using Daiwanese only for MakerDAO-facing features, as this was a requirement from Coinbase.

The structure of the 2 step ownership makes checking for address(0) edge cases mandatory however allows for smart contract wallets / recipients. This is a contextual thing, I want to confirm that this is intended.

For this specific deal, the recipient is going to be a smart contract (a RwaSwapInputConduit2 instance to be more precise). I don't think adding a block for EOAs brings much in terms of security, because changing a recipient requires collaboration between both parties. The chances of having this being set to an unintended address is pretty slim, since changes have to go through the due legal process.

IsOwner and IsCustodian seem redundant unless I'm missing some context here.

These are mainly for Coinbase's use. While the custodians mapping has a "common" name, they felt the need of having something closer to the legal language "owner", so we added a isOwner as a wrapper for wards. For consistency, we decided to add isCustodian, but the latter can be removed if PE feels strongly against it.

Comments and NATSPEC could do with trimming down depending on how dss-ish you want the code to look; this resembles how I wrote my own code prior to adapting to the dss style so this is just a general comment.

Our understanding is that this is not a Core MCD contract, so it would be fine to use CES coding convention here. Again, it's not something that would be a blocker for us, however considering the Coinbase requirements, I find it very hard to make it 100% compliant anyways.

I would appreciate some notes on why address(0) is checked for (i.e. to allow pendingRecipient to work only if it's initialised)

Noted! Will update the docs.

Not immediately clear to me how the Abstracts are used with the external view return functions in SafeBoxLike.sol but this is probably my lack of knowledge (have never tried to just return an imported interface before). Knowledge gap here so someone else needs to fill this in and make sure it's OK.

From an ABI perspective, those types are simply translated to address. However, we usually store contract references with their actual type to make them easier to consume. It's out of the scope of this review, but the contract to enable the recovery of funds after Emergency Shutdown will use the SafeboxLike interface.

VatAbstract and GemAbstract OK - do we really need to use dss interfaces for only 3 interfaces? Personally I would use VatLike and GemLike.

Those interfaces are pretty much set in stone at this point. We import the specific files directly, so it wouldn't be a problem of bloating contract source verification for instance. We could make some copy-pasta from dss-interfaces and remove the methods not used though if you think that's the best way.

onlyCustodian code OK - modifier name seems a bit out of place since it sits next to auth - not a big issue

I've been back and forth with the layout of this contract a couple of times TBH 😅.
I'll probably move everything accessible only to coinbase to their designated section.

Existing implementation seems OK as it is. The benefit of it is that it doesn't rely on a call from the recipient's address. The drawback is that we have to ensure that the recipient isn't address(0) for this to be possible.

Yes, this is by design. Recipient is not meant to play an active role here because.

Regarding the possible attack vector:

Assume that I am a malicious member of governance and decide to steal the funds of the SafeBox. I can use CREATE2crunch or another similar tool to determine the salt for a contract address which resembles an ERC20 token's address, then deploy my own ERC20 there which looks like the real thing, however, my transfer function is malicious.
While a deposit function would only transfer in tokens with address token (meaning that deposit and withdraw keeps a 'closed loop' such that the deposited token is the one being withdrawn), depositing directly to the contract would allow this kind of attack to occur if there was someone malicious responsible for the SafeBox deployment.

Wouldn't this be solved by having the diligence of checking the token value matches exactly the expected one after deployment before the onboarding spell?

We can even safeguard against this in the spell itself by having a sanity check like:

require(address(safebox.token()) == chainlog.getAddress("<TOKEN>"), "Invalid safebox token");

Any other asset sent to this contract other than token itself is locked there forever.

amusingaxl · 2022-11-10T20:04:49Z

@amusingaxl can you please point us to the code of the input conduit with the support for post shutdown collateral distribution (basically the intended recipient of SafeBox)?

I'm finishing the required changes and will post the link to it here as soon as I'm done.

amusingaxl · 2022-11-10T21:13:49Z

@The-Arbiter @rockyfour here's the input conduit that is going to be used as recipient in this case: https://github.com/makerdao/mip21-toolkit/pull/27/files#diff-42d74a78f57f1a5daa3af319c8bdbec3e7850b836cd393fb7caa0b3ada65a0e2

Adds custodial deny

feat: introduce safebox contracts

86f176b

amusingaxl requested review from nduchak and dbkbali October 25, 2022 14:45

amusingaxl added 10 commits October 25, 2022 12:20

docs: add README

08a6e97

docs: improve README with usage details

a44a111

chore: update editorconfig

513cfd3

chore: update package.json dev dependencies

8df8105

chore: update solidity plugin for prettier

ee9619a

refactor: make safebox permissions immutable

a82cb33

docs: update README to reflect the latest changes

a6811f9

chore: specify solc version in foundry config

7f2a015

docs: improve README structure

1b892ff

docs: fix typo on change recipient address description

4386d7a

dbkbali approved these changes Nov 3, 2022

View reviewed changes

nduchak approved these changes Nov 3, 2022

View reviewed changes

amusingaxl added 4 commits November 4, 2022 12:14

docs: fix outdated natspec

b904603

docs: improve wording on custodian role description

d4a683a

refactor: make the safebox ready for emergency shutdown

fd21d07

chore: enable CI actions

0a06177

amusingaxl requested review from dbkbali and nduchak November 8, 2022 19:20

dbkbali suggested changes Nov 8, 2022

View reviewed changes

README.md Show resolved Hide resolved

gbalabasquer reviewed Nov 9, 2022

View reviewed changes

rockyfour reviewed Nov 9, 2022

View reviewed changes

refactor: add the ability to custodians to add or remove other custod…

809529a

…ians

rockyfour reviewed Nov 10, 2022

View reviewed changes

amusingaxl added 2 commits November 10, 2022 11:32

refactor: rename RecipientChange event to SetRecipient

d72f7f1

refactor: remove deposit function

f45d28d

`deposit` will most likely never be used, so we are removing it.

amusingaxl requested review from rockyfour and removed request for gbalabasquer November 10, 2022 15:21

amusingaxl added 2 commits November 10, 2022 12:22

refactor: reorganize SafeboxRecovery constructor

7d311f7

refactor: remove SafeboxRecovery from the scope of this PR

4b411b5

The-Arbiter reviewed Nov 10, 2022

View reviewed changes

refactor: address review comments by @The-Arbiter

05211af

azmenak and others added 12 commits November 22, 2022 10:30

Adds custodial deny

5e8962b

Emits deny event

65f554c

Remove default withdrawl method

d5d63eb

Merge pull request #2 from azmenak/feat/safebox-contracts-cb-custody

77e728d

Adds custodial deny

chore: update forge-std

e99f483

refactor: improve timelock implementation

a030c6f

docs: add timelock documentation

18b8b57

refactor: add missing test cases

24b2221

feat: add fast withdrawal execution for custodians

dc1d4c8

fix: typo on revert reason

0674afc

feat: prevent withdrawal denial after timelock

1c485bb

feat: implement additional Emergency Shutdown use cases

fe3189e

	mapping(address => uint256) public override wards;
	mapping (address => uint256) public override wards;

	function rely(address usr) external override auth {
	function rely(address usr) external auth {

	function deny(address usr) external override auth {
	function deny(address usr) external auth {

	function file(bytes32 what, address data) external override auth {
	function file(bytes32 what, address data) public auth {

Feat: introduce Safebox contracts #1

Are you sure you want to change the base?

Feat: introduce Safebox contracts #1

Conversation

amusingaxl commented Oct 25, 2022 • edited Loading

dbkbali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amusingaxl Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The-Arbiter left a comment

Choose a reason for hiding this comment

TLDR

Documentation

SafeBox.sol

Attack Vectors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The-Arbiter Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockyfour commented Nov 10, 2022

amusingaxl commented Nov 10, 2022

amusingaxl commented Nov 10, 2022

amusingaxl commented Nov 10, 2022

Feat: introduce `Safebox` contracts #1

Feat: introduce `Safebox` contracts #1

amusingaxl commented Oct 25, 2022 •

edited

Loading

amusingaxl Nov 10, 2022 •

edited

Loading

The-Arbiter Nov 10, 2022 •

edited

Loading