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

Add allowed callbacks proposal #2

Merged
merged 4 commits into from
Jul 10, 2018

Conversation

AugustoL
Copy link
Contributor

@AugustoL AugustoL commented Jul 4, 2018

This is a proposal of changes to ERC827 standard to change the way callbacks works. This fix the issue of undesired callbacks being use in certain contracts.

Summary

The receiver contract where the callback function will be executed now needs to allow the execution of that callback itself, if the sender attempt to execute a callback that is not allowed on a receiver contract it will fail.
The callbacks can be allowed to certain address or they can be public, which means that anyone can execute it. There is also the option for a receiver contract to allow the execution of any function from any address.

The allowedCallbacks mapping works like this:

Receiver => Sender => Function Signature => FunctionType => Allowed

The receiver is the contract where the callbak function will be executed.

The sender can be an specific address or it can be 0x0, if it is 0x0 means that is a public callback.

The function signature can be an specific signature or it can be 0x0, if it is 0x0 means that any function can be executed.

The function type can be 1(approval), 2(transfer) or 3(transferFrom).

The allowed variable is a boolean that is true or false depending if the callback is allowed by the sender or not.

To manage the allowance of the callback the proposal adds two new functions:

function allowCallback(address from, bytes4 functionSignature, uint8 fType) public
function removeCallback(address from, bytes4 functionSignature, uint8 fType) public

See the ERC827TokenAllowedCallbacks

Backwards compatibility

To work with smart contracts that didnt had werent able to allow specific callbacks to be executed I added a ERC827Proxy contract that forwards any arbitrary call with token balance or allowance. This contract only needs to be deployed specifying the token address and it will automatically allow the makeCall function to be executed from the token contract.

The PR also add tests for the proposed changes.

Copy link

@alvinveroy alvinveroy left a comment

Choose a reason for hiding this comment

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

Should the receiver contract also need to explicitly allow the erc827 callback?

Copy link

@Perseverance Perseverance left a comment

Choose a reason for hiding this comment

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

The more I think about it, the more I like it. The general use-case from my standpoint is allowCallback('0x', functionSignature).

On the other side, the main problem left with this solution would be working with already existing contracts. For example the DEX platform will not be able to use the goodies of approveAndCall and transferAndCall as it has no mechanism to trigger the allowCallback (if it is not upgradeable contract ofc). Maybe the solution that @k06a proposed is an answer to this issue?

@AugustoL
Copy link
Contributor Author

AugustoL commented Jul 6, 2018

@Perseverance they would still be able to use the ERC20 methods, and any new contract will be able to use the ERC827 methods.
One thing that is missing is the allowance per type of function, If as a receiver contract I want only the callback to be executed in approveAndCall for example.

Copy link

@Perseverance Perseverance left a comment

Choose a reason for hiding this comment

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

Should we here have some kind of check that _functionSignature is the same function _data is going to trigger. For example, can I pass as _functionSignature an approved function, but target tokenFallback using the _data?

function approveAndCall(
address _spender,
uint256 _value,
bytes4 _functionSignature,

Choose a reason for hiding this comment

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

Let's not rely on the sender to provide the signature, but instead take the first 4 bytes of the data.

Here is a quick way to extract them.

bytes4 signature;

assembly {
       signature := mload(add(_data, 32))
}

@@ -14,42 +14,59 @@ import "../../ERC20/StandardToken.sol";
*/
contract ERC827TokenAllowedCallbacks is StandardToken {

// @dev Enum to represent the type of token functions in uint8
enum FunctionType { None, Approve, Transfer, TransferFrom }

Choose a reason for hiding this comment

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

Does it make sense to add another type All?

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 had it when I first implemented it, but I dont know how necessary it is, I think the callbacks allowance should be as restrictive as It can, I dont see the option "All" being used that much, and I dont know how safe it is to use it. I see approval as the most important one for callbacks and more secure, and this is the one that should be only used if you can.
It also make more complex the check for callbacks allowed (and this is executed in every ERC827 function).
If you really want to allow callback for all functions it will be calling allowCallbak function three times instead of one.
But again, I wont reccomend allowing callbacks for all functions at least you are 100% sure that is safe.

@AugustoL AugustoL force-pushed the allowed-callbacks-proposal branch from 864df1f to caedc63 Compare July 9, 2018 10:07
@AugustoL AugustoL changed the title [WIP] Add allowed callbacks proposal Add allowed callbacks proposal Jul 10, 2018
@AugustoL AugustoL merged commit e574b35 into windingtree:master Jul 10, 2018
@AdelaDella
Copy link

现在有一个完善的 827合约吗

@AugustoL AugustoL deleted the allowed-callbacks-proposal branch November 13, 2018 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants