Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agent: Merge with Fundraising's Pool #941

Merged
merged 26 commits into from Aug 12, 2019

Conversation

@osarrouy
Copy link
Contributor

commented Jul 31, 2019

Fixes #656.

@welcome

This comment has been minimized.

Copy link

commented Jul 31, 2019

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

@coveralls

This comment has been minimized.

Copy link

commented Jul 31, 2019

Coverage Status

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

@izqui
Copy link
Member

left a comment

Looking really good!

// 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);

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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);

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

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


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

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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.

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

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].

* @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);

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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

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`

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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

* @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) {

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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.

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

Sounds good.

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);

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

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.

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

_removeProtectedToken(_token);

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member
Suggested change
_removeProtectedToken(_token);
_removeProtectedToken(_token);
*/
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);

This comment has been minimized.

Copy link
@izqui

izqui Aug 1, 2019

Member

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.

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

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 ...

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 2, 2019

Author Contributor

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

  1. 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].

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

This comment has been minimized.

Copy link
@izqui

izqui Aug 5, 2019

Member

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 Outdated Show resolved Hide resolved

@sohkai sohkai removed the request for review from bpierre Aug 2, 2019

@osarrouy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

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].

@osarrouy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

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

})

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

This comment has been minimized.

Copy link
@izqui

izqui Aug 5, 2019

Member

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

})

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

This comment has been minimized.

Copy link
@izqui

izqui Aug 5, 2019

Member

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

*/
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);

This comment has been minimized.

Copy link
@izqui

izqui Aug 5, 2019

Member
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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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

@izqui

izqui approved these changes Aug 7, 2019

sohkai added some commits Aug 8, 2019

sohkai added some commits Aug 8, 2019

@osarrouy

This comment has been minimized.

Copy link

commented on apps/agent/contracts/Agent.sol in 14576ae Aug 9, 2019

Do you also get gas refund that way ?

EDIT. Answered off-github: yes :)

@sohkai

sohkai approved these changes Aug 9, 2019

Copy link
Member

left a comment

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

izqui approved these changes Aug 12, 2019

Copy link
Member

left a comment

Amazing work @sohkai. LGTM!

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

3 of 4 checks passed

Travis CI - Pull Request Build Failed
Details
WIP Ready for review
Details
coverage/coveralls Coverage increased (+0.5%) to 98.488%
Details
license/cla Contributor License Agreement is signed.
Details
@welcome

This comment has been minimized.

Copy link

commented Aug 12, 2019

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.