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.

Sign up for GitHub

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
Merged

Agent: Merge with Fundraising's Pool #941

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3facd3f
Merge Pool into Agent
osarrouy Jul 31, 2019
7b3d92f
Update roles
osarrouy Aug 2, 2019
1749731
Fix indentation issue
osarrouy Aug 2, 2019
c7fa7e9
Allow protected balance increase
osarrouy Aug 2, 2019
87db2c5
Cache protectedTokens.length and check it has not been altered during…
osarrouy Aug 2, 2019
365298e
Update radspec description
osarrouy Aug 2, 2019
26aa1c2
Update SAFE_EXECUTE_ROLE to be permissioned
osarrouy Aug 2, 2019
1c262dd
Remove use of ETH as a fake ERC20
osarrouy Aug 2, 2019
05784df
Move ERC20 check into its own function
osarrouy Aug 2, 2019
0aa07c6
Update tests
osarrouy Aug 2, 2019
6512c27
Rename _protectedTokens into protectedTokens_
osarrouy Aug 7, 2019
4e8f3ff
Add new test cases
osarrouy Aug 7, 2019
c069ff1
Add additionnal ERC20 checks
osarrouy Aug 7, 2019
63a0f83
Agent: reorder mainnet environment to be more prominent, remove old r…
sohkai Aug 8, 2019
99c793f
Agent: cosmetic reordering of external functions and roles in arapp
sohkai Aug 8, 2019
2946020
Agent: cosmetic reordering of errors for usage
sohkai Aug 8, 2019
a4a97b2
Agent: cosmetic reordering of functions and conforming to code style …
sohkai Aug 8, 2019
1f6c9f9
Agent: simplify forwarding permission check
sohkai Aug 8, 2019
3ac568c
Agent: cosmetic comment clarifications
sohkai Aug 9, 2019
14576ae
Agent: small code optimizations
sohkai Aug 9, 2019
22392f7
Agent: add token as parameter to add and remove protected token roles
sohkai Aug 9, 2019
aa05a88
Agent: add getter for length of protected tokens
sohkai Aug 9, 2019
eb03d9c
Agent: cosmetic reorganization of tests
sohkai Aug 9, 2019
9310cb6
Agent: improve tests with revert strings, more checks
sohkai Aug 9, 2019
1e77045
Agent: add uninitalized tests
sohkai Aug 9, 2019
e47298f
Merge pull request #1 from AragonBlack/agent-changes
Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions apps/agent/contracts/Agent.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,100 @@ import "@aragon/os/contracts/common/IForwarder.sol";


contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
/* Hardcoded constants to save gas
bytes32 public constant SAFE_EXECUTE_ROLE = keccak256("SAFE_EXECUTE_ROLE");
bytes32 public constant EXECUTE_ROLE = keccak256("EXECUTE_ROLE");
bytes32 public constant RUN_SCRIPT_ROLE = keccak256("RUN_SCRIPT_ROLE");
bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = keccak256("ADD_PROTECTED_TOKEN_ROLE");
bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = keccak256("REMOVE_PROTECTED_TOKEN_ROLE");
bytes32 public constant ADD_PRESIGNED_HASH_ROLE = keccak256("ADD_PRESIGNED_HASH_ROLE");
bytes32 public constant DESIGNATE_SIGNER_ROLE = keccak256("DESIGNATE_SIGNER_ROLE");
*/

bytes32 public constant SAFE_EXECUTE_ROLE = 0x0a1ad7b87f5846153c6d5a1f761d71c7d0cfd122384f56066cd33239b7933694;
sohkai marked this conversation as resolved.
Show resolved Hide resolved
bytes32 public constant EXECUTE_ROLE = 0xcebf517aa4440d1d125e0355aae64401211d0848a23c02cc5d29a14822580ba4;
bytes32 public constant RUN_SCRIPT_ROLE = 0xb421f7ad7646747f3051c50c0b8e2377839296cd4973e27f63821d73e390338f;
bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = 0x6eb2a499556bfa2872f5aa15812b956cc4a71b4d64eb3553f7073c7e41415aaa;
bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = 0x71eee93d500f6f065e38b27d242a756466a00a52a1dbcd6b4260f01a8640402a;
bytes32 public constant ADD_PRESIGNED_HASH_ROLE = 0x0b29780bb523a130b3b01f231ef49ed2fa2781645591a0b0a44ca98f15a5994c;
bytes32 public constant DESIGNATE_SIGNER_ROLE = 0x23ce341656c3f14df6692eebd4757791e33662b7dcf9970c8308303da5472b7c;

uint256 public constant PROTECTED_TOKENS_CAP = 10;

bytes4 private constant ERC165_INTERFACE_ID = 0x01ffc9a7;

string private constant ERROR_TOKENS_CAP_REACHED = "AGENT_TOKENS_CAP_REACHED";
string private constant ERROR_TOKEN_NOT_ETH_OR_CONTRACT = "AGENT_TOKEN_NOT_ETH_OR_CONTRACT";
string private constant ERROR_TOKEN_ALREADY_PROTECTED = "AGENT_TOKEN_ALREADY_PROTECTED";
string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
string private constant ERROR_PROTECTED_TOKENS_MODIFIED = "AGENT_PROTECTED_TOKENS_MODIFIED";
string private constant ERROR_BALANCE_NOT_CONSTANT = "AGENT_BALANCE_NOT_CONSTANT";
string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";

mapping (bytes32 => bool) public isPresigned;
address public designatedSigner;
address[] public protectedTokens;

event SafeExecute(address indexed sender, address indexed target, bytes data);
event Execute(address indexed sender, address indexed target, uint256 ethValue, bytes data);
event AddProtectedToken(address indexed token);
event RemoveProtectedToken(address indexed token);
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`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _target Address where the action is being executed
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

address[] memory _protectedTokens = new address[](protectedTokens.length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

uint256[] memory balances = new uint256[](protectedTokens.length);
bytes32 size;
bytes32 ptr;

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// we copy the protected tokens array to check whether the storage array has been modified during the underlying call
_protectedTokens[i] = token;
// we copy the balances to check whether they have been modified during the underlying call
balances[i] = balance(token);
}

bool result = _target.call(_data);

assembly {
size := returndatasize
ptr := mload(0x40)
mstore(0x40, add(ptr, size))
returndatacopy(ptr, 0, size)
}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

emit SafeExecute(msg.sender, _target, _data);

assembly {
return(ptr, size)
}
} else {
// if the underlying call has failed, we revert and forward [possible] returned error data
assembly {
revert(ptr, size)
}
}
}

/**
* @notice Execute '`@radspec(_target, _data)`' on `_target``_ethValue == 0 ? '' : ' (Sending' + @tokenAmount(0x0000000000000000000000000000000000000000, _ethValue) + ')'`
* @param _target Address where the action is being executed
Expand Down Expand Up @@ -59,6 +137,28 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
}
}

/**
* @notice Add `_token.symbol(): string` to the list of protected tokens
* @param _token Address of the token to be protected
*/
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

require(!tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);

_addProtectedToken(_token);
}

/**
* @notice Remove `_token.symbol(): string` from the list of protected tokens
* @param _token Address of the token to be unprotected
*/
function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
require(tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);

_removeProtectedToken(_token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_removeProtectedToken(_token);
_removeProtectedToken(_token);

}

/**
* @notice Set `_designatedSigner` as the designated signer of the app, which will be able to sign messages on behalf of the app
* @param _designatedSigner Address that will be able to sign messages on behalf of the app
Expand Down Expand Up @@ -141,6 +241,20 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
return canPerform(sender, RUN_SCRIPT_ROLE, params);
}

function _addProtectedToken(address _token) internal {
protectedTokens.push(_token);

emit AddProtectedToken(_token);
}

function _removeProtectedToken(address _token) internal {
protectedTokens[protectedTokenIndex(_token)] = protectedTokens[protectedTokens.length - 1];
delete protectedTokens[protectedTokens.length - 1];
protectedTokens.length --;

emit RemoveProtectedToken(_token);
}

function getScriptACLParam(bytes evmScript) internal pure returns (uint256) {
return uint256(keccak256(abi.encodePacked(evmScript)));
}
Expand All @@ -152,4 +266,24 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {

assembly { sig := mload(add(data, 0x20)) }
}

function tokenIsProtected(address _token) internal view returns (bool) {
for (uint256 i = 0; i < protectedTokens.length; i++) {
if (protectedTokens[i] == _token) {
return true;
}
}

return false;
}

function protectedTokenIndex(address _token) internal view returns (uint256) {
for (uint i = 0; i < protectedTokens.length; i++) {
if (protectedTokens[i] == _token) {
return i;
}
}

revert(ERROR_TOKEN_NOT_PROTECTED);
}
}
6 changes: 6 additions & 0 deletions apps/agent/contracts/test/mocks/ExecutionTarget.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
pragma solidity 0.4.24;

import "@aragon/test-helpers/contracts/TokenMock.sol";


contract ExecutionTarget {
uint public counter;
Expand All @@ -15,6 +17,10 @@ contract ExecutionTarget {
counter = x;
}

function transferTokenFrom(address _token) external {
TokenMock(_token).transferFrom(msg.sender, this, 1);
}

function fail() external pure {
revert(REASON);
}
Expand Down
Loading