Agent: Merge with Fundraising's Pool #941

osarrouy · 2019-07-31T10:27:18Z

Fixes #656.

welcome · 2019-07-31T10:27:21Z

Thanks for opening this pull request! Someone will review it soon 🔍

coveralls · 2019-07-31T13:56:26Z

Coverage increased (+0.5%) to 98.488% when pulling e47298f on AragonBlack:master into 8aa28e0 on aragon:master.

izqui

Looking really good!

izqui · 2019-07-31T13:45:40Z

apps/agent/contracts/Agent.sol

+            // and their balances have not been modified and return the call's return data
+            for (uint256 j = 0; j < _protectedTokens.length; j++) {
+                require(protectedTokens[j] == _protectedTokens[j], ERROR_PROTECTED_TOKENS_MODIFIED);
+                require(balances[j] == balance(_protectedTokens[j]), ERROR_BALANCE_NOT_CONSTANT);


What do you think about allowing the token balance to increase when safe executing? It could be useful for use cases such as claiming rewards.

Suggested change

require(balances[j] == balance(_protectedTokens[j]), ERROR_BALANCE_NOT_CONSTANT);

require(balance(_protectedTokens[j] >= balances[j]), ERROR_BALANCE_NOT_CONSTANT);

I think it makes sense indeed ... Gonna update that.

izqui · 2019-07-31T13:55:48Z

apps/agent/contracts/Agent.sol

+
+        if (result) {
+            // if the underlying call has succeeded, we check that the protected tokens
+            // and their balances have not been modified and return the call's return data


Perhaps let's require that _protectedTokens.length == protectedTokens.length. I don't think this can be exploited, but we could fail with a nicer error than accessing an out of bounds item in the array.

Yeah. This could also allow us to make sure no protected tokens has been added [though i'm not sure this could be an attack vector].

izqui · 2019-07-31T13:56:24Z

apps/agent/contracts/Agent.sol

+    * @return Exits call frame forwarding the return data of the executed call (either error or success data)
+    */
+    function safeExecute(address _target, bytes _data) external auth(SAFE_EXECUTE_ROLE) {
+        address[] memory _protectedTokens = new address[](protectedTokens.length);


We could cache protectedTokens.length into a stack variable so we don't read it from storage every time.

izqui · 2019-07-31T13:59:31Z

apps/agent/contracts/Agent.sol

    event PresignHash(address indexed sender, bytes32 indexed hash);
    event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);

+    /**
+    * @notice Safe execute '`@radspec(_target, _data)`' on `_target`


Maybe let's have a more clear description of what the function does.

Suggested change

* @notice Safe execute '`@radspec(_target, _data)`' on `_target`

* @notice Execute '`@radspec(_target, _data)`' on `_target` (without allowing transfers of protected tokens)

Not super convinced about the suggestion, but I would go for something like this

izqui · 2019-07-31T17:05:53Z

apps/agent/contracts/Agent.sol

+    * @param _data Calldata for the action
+    * @return Exits call frame forwarding the return data of the executed call (either error or success data)
+    */
+    function safeExecute(address _target, bytes _data) external auth(SAFE_EXECUTE_ROLE) {


I would add the same parameters to the role as EXECUTE_ROLE has. For _value it could either be always 0 or we could remove it as a parameter altogether.

Sounds good.

izqui · 2019-07-31T17:07:19Z

apps/agent/contracts/Agent.sol

+        for (uint256 i = 0; i < protectedTokens.length; i++) {
+            address token = protectedTokens[i];
+            // we don't care if target is ETH [0x00...0] as it can't be spent anyhow [though you can't invoke anything at 0x00...0]
+            require(_target != token || token == ETH, ERROR_TARGET_PROTECTED);


What's the rationale behind allowing to add ETH to the protected token list?

Well it was mostly a convenient thing for us regarding our use case: basically the Controller could then expose an addCollateralToken function which would forward the call to MarketMaker, Tap and Pool [no matter whether this is an ERC20 or ETH].

You're right that it does not make a lot of sense into the Agent as such. I will update it and handle this specific use case in the Controller contract.

izqui · 2019-07-31T17:11:00Z

apps/agent/contracts/Agent.sol

+    function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
+        require(tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
+
+      _removeProtectedToken(_token);


Suggested change

_removeProtectedToken(_token);

_removeProtectedToken(_token);

izqui · 2019-08-01T11:27:16Z

apps/agent/contracts/Agent.sol

+    */
+    function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
+        require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
+        require(isContract(_token) || _token == ETH, ERROR_TOKEN_NOT_ETH_OR_CONTRACT);


What if in addition to ensuring that _token is a contract, we perform a balance(_token) to make sure that the balance check doesn't revert for the token. If a token is added and token.balanceOf() reverts, it will block safeExecute until the token is removed from the protected list.

A malicious token could still be added that starts reverting after a state change or some period of time. To prevent this we could add a public function (without authentication) to remove a token that reverts, but this is a bit of an overkill IMO.

What if in addition to ensuring that _token is a contract, we perform a balance(_token) to make sure that the balance check doesn't revert for the token. If a token is added and token.balanceOf() reverts, it will block safeExecute until the token is removed from the protected list

Hmmmm. Yeah. I will add that too.

A malicious token could still be added that starts reverting after a state change or some period of time. To prevent this we could add a public function (without authentication) to remove a token that reverts, but this is a bit of an overkill IMO.

Yeah I think it will mostly make things harder to understand [regarding roles and all] for end users while the treat is really low ...

Thinking about it: what exactly is the threat model here?

If we are trying to deal with malicious tokens : what about the situation where calling balanceOf would actually empty your balance ? It's unlikely cause if the token is malicious / not trustless it could directly move your tokens [which would be pointless cause they would enp be worth nothing in the end].

We are just trying to prevent users to do a mistake while copy / pasting the address of the token ?

For me the threat model is adding a contract to the list of protected tokens that can brick safeExecute until the token is removed. This could happen by mistake or maliciously and it could be specially bad if the permission to remove a token from the list is assigned to a long/complicated process or even burned.

But as you point out, adding a malicious token could bypass any check that we add here, so we would really just be protecting from mistakes.

what about the situation where calling balanceOf would actually empty your balance ?

This shouldn't even be a problem since balance(_token) uses a STATICCALL.

apps/agent/contracts/Agent.sol

osarrouy · 2019-08-02T14:58:46Z

I just updated the roles in the arapp.json [cause I forgot before]. I also took the freedom to add the WITHDRAW_ROLE inherited from the Vault which otherwise won't appear in the client permissions list [or will be unreadable].

… script execution

osarrouy · 2019-08-02T16:10:19Z

@izqui All comments should be fixed now [excepts for the ERC20 check thing cause i'm waiting for your answer on that].

izqui · 2019-08-05T08:47:00Z

apps/agent/test/agent_shared.js

+        })
+
+        context('> and target is not a protected ERC20 but action affects a protected ERC20 balance', () => {
+          it('it should revert', async () => {


We could add a test case for the Agent's balance increasing as a result of the execution.

izqui · 2019-08-05T08:48:09Z

apps/agent/test/agent_shared.js

+        })
+
+        context('> and target is not a protected ERC20 but action affects a protected ERC20 balance', () => {
+          it('it should revert', async () => {


We should also test the execution modifying the protected token list which should cause a revert.

izqui · 2019-08-05T08:50:20Z

apps/agent/contracts/Agent.sol

+    */
+    function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(getSig(_data)))) {
+        uint256 protectedTokensLength = protectedTokens.length;
+        address[] memory _protectedTokens = new address[](protectedTokensLength);


Suggested change

address[] memory _protectedTokens = new address[](protectedTokensLength);

address[] memory protectedTokens_ = new address[](protectedTokensLength);

We normally use this notation when the variable is not an argument to the function (and we need to add an _)

osarrouy · 2019-08-07T11:12:37Z

Added all the test cases plus some additionnal checks for ERC20.

For now the contract just calls balance on the token to check if the added token is an ERC20. The result is that if the token is not an ERC20 the error message is SAFE_ERC_20_BALANCE_REVERTED.

Do you think it's worth making a low level call to catch the error and return a clearer error message like AGENT_PROTECTED_TOKEN_NOT_ERC20 ?

izqui · 2019-08-07T16:09:15Z

@osarrouy I don't think it is worth it to do the low-level call, that error is fine IMO.

…inkeby environment

…in other apps

Agent enhancements

sohkai

✅ I've just added a number of commits to make the code style consistent with the rest of the apps, improve some of the tests, and add minor optimizations / features.

It would be great if you could take a quick look again through the new commits @izqui!

izqui

Amazing work @sohkai. LGTM!

welcome · 2019-08-12T20:23:17Z

Congrats on merging your first pull request! Aragon is proud of you 🦅

Merge Pool into Agent

3facd3f

probot-autolabeler bot added app: agent component: contract labels Jul 31, 2019

auto-assign bot requested review from bpierre and sohkai July 31, 2019 10:27

izqui reviewed Aug 1, 2019

View reviewed changes

sohkai removed the request for review from bpierre August 2, 2019 10:29

Update roles

7b3d92f

osarrouy added 8 commits August 2, 2019 17:17

Fix indentation issue

1749731

Allow protected balance increase

c7fa7e9

Cache protectedTokens.length and check it has not been altered during…

87db2c5

… script execution

Update radspec description

365298e

Update SAFE_EXECUTE_ROLE to be permissioned

26aa1c2

Remove use of ETH as a fake ERC20

1c262dd

Move ERC20 check into its own function

05784df

Update tests

0aa07c6

sohkai mentioned this pull request Aug 2, 2019

Review Agent's changes from Pool aragonone/product#163

Closed

izqui reviewed Aug 5, 2019

View reviewed changes

osarrouy added 3 commits August 7, 2019 12:20

Rename _protectedTokens into protectedTokens_

6512c27

Add new test cases

4e8f3ff

Add additionnal ERC20 checks

c069ff1

izqui approved these changes Aug 7, 2019

View reviewed changes

sohkai added 2 commits August 8, 2019 22:55

Agent: reorder mainnet environment to be more prominent, remove old r…

63a0f83

…inkeby environment

Agent: cosmetic reordering of external functions and roles in arapp

99c793f

sohkai and others added 11 commits August 8, 2019 23:08

Agent: cosmetic reordering of errors for usage

2946020

Agent: cosmetic reordering of functions and conforming to code style …

a4a97b2

…in other apps

Agent: simplify forwarding permission check

1f6c9f9

Agent: cosmetic comment clarifications

3ac568c

Agent: small code optimizations

14576ae

Agent: add token as parameter to add and remove protected token roles

22392f7

Agent: add getter for length of protected tokens

aa05a88

Agent: cosmetic reorganization of tests

eb03d9c

Agent: improve tests with revert strings, more checks

9310cb6

Agent: add uninitalized tests

1e77045

Merge pull request #1 from AragonBlack/agent-changes

e47298f

Agent enhancements

sohkai approved these changes Aug 9, 2019

View reviewed changes

izqui approved these changes Aug 12, 2019

View reviewed changes

sohkai merged commit e66c06f into aragon:master Aug 12, 2019

sohkai changed the title ~~Merge Pool into Agent~~ Agent: Merge with Fundraising's Poo Aug 12, 2019

sohkai changed the title ~~Agent: Merge with Fundraising's Poo~~ Agent: Merge with Fundraising's Pool Aug 13, 2019

facuspagnuolo pushed a commit that referenced this pull request Sep 3, 2019

Agent: Merge with Fundraising's Pool (#941)

f059f17

ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021

Agent: Merge with Fundraising's Pool (aragon#941)

7ff09b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agent: Merge with Fundraising's Pool #941

Agent: Merge with Fundraising's Pool #941

osarrouy commented Jul 31, 2019 •

edited by sohkai

Loading

welcome bot commented Jul 31, 2019

coveralls commented Jul 31, 2019 •

edited

Loading

izqui left a comment

izqui Jul 31, 2019

osarrouy Aug 2, 2019

izqui Jul 31, 2019

osarrouy Aug 2, 2019

izqui Jul 31, 2019

izqui Jul 31, 2019

izqui Jul 31, 2019

osarrouy Aug 2, 2019

izqui Jul 31, 2019

osarrouy Aug 2, 2019

izqui Jul 31, 2019

izqui Aug 1, 2019

osarrouy Aug 2, 2019

osarrouy Aug 2, 2019

izqui Aug 5, 2019

osarrouy commented Aug 2, 2019

osarrouy commented Aug 2, 2019

izqui Aug 5, 2019

izqui Aug 5, 2019

izqui Aug 5, 2019

osarrouy commented Aug 7, 2019

izqui commented Aug 7, 2019

sohkai left a comment

izqui left a comment

welcome bot commented Aug 12, 2019

	require(balances[j] == balance(_protectedTokens[j]), ERROR_BALANCE_NOT_CONSTANT);
	require(balance(_protectedTokens[j] >= balances[j]), ERROR_BALANCE_NOT_CONSTANT);

	* @notice Safe execute '`@radspec(_target, _data)`' on `_target`
	* @notice Execute '`@radspec(_target, _data)`' on `_target` (without allowing transfers of protected tokens)

	_removeProtectedToken(_token);
	_removeProtectedToken(_token);

	address[] memory _protectedTokens = new address[](protectedTokensLength);
	address[] memory protectedTokens_ = new address[](protectedTokensLength);

Agent: Merge with Fundraising's Pool #941

Agent: Merge with Fundraising's Pool #941

Conversation

osarrouy commented Jul 31, 2019 • edited by sohkai Loading

welcome bot commented Jul 31, 2019

coveralls commented Jul 31, 2019 • edited Loading

izqui 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

osarrouy commented Aug 2, 2019

osarrouy commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osarrouy commented Aug 7, 2019

izqui commented Aug 7, 2019

sohkai left a comment

Choose a reason for hiding this comment

izqui left a comment

Choose a reason for hiding this comment

welcome bot commented Aug 12, 2019

osarrouy commented Jul 31, 2019 •

edited by sohkai

Loading

coveralls commented Jul 31, 2019 •

edited

Loading