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

Track @next contract changes (Aragon 0.7-part.2) #723

Merged
merged 27 commits into from Apr 10, 2019

Conversation

Projects
None yet
3 participants
@sohkai
Copy link
Member

sohkai commented Mar 20, 2019

Completed security review.

sohkai added some commits Mar 20, 2019

Survey: optimize voteOption() (#705)
Optimizes `_voteOption()` by caching the `MultiOptionVote` storage variable, rather than recalculating its position multiple times.
Finance: add uint256 casts for parameters in authP (#708)
Although not strictly necessary, we use these casts in other places where we convert from a non-uint256 type to be a parameter in `authP()`.
Token Manager: remove unused storage mapping (#712)
`everHeld` was never used and luckily held the last storage slot, so we can safely pretend it never existed.
Finance, Token Manager, Voting: split utility getters into public and…
… internal functions with different protection schemes (#702)

Ensure all public utility functions requiring valid state enforce initialization checks.

Unsafe internal variants of these functions are provided that bypass the initialization check for optimized use inside the contract.
Finance: fix naming of status parameter in ChangePaymentState event (#…
…707)

The event is actually being [emitted with whether the payment is active or not](https://github.com/aragon/aragon-apps/blob/master/apps/finance/contracts/Finance.sol#L351).

This is a breaking change for frontend clients relying on this parameter name, but to my knowledge nobody is using this parameter.

@aragon aragon deleted a comment from coveralls Mar 20, 2019

sohkai added some commits Mar 21, 2019

sohkai added some commits Mar 28, 2019

Voting: mock time in tests rather than use timetravel (#737)
Must've forgot to do this back when we migrated to aragonOS@4 😅. `timeTravel()` isn't supported outside of Ganache, so we generally use the built-in time mocks in `AragonApp`.
Finance, Survey, Token Manager, Voting: Clarify internal protection s…
…cheme (#710)

Builds on #702.

An attempt at an answer to #686 (comment).

This will be added as a guideline to hack.aragon.org (soon), but in short, my current thoughts on where to place security modifiers in Aragon apps:

- "Lowest" in the call stack:
  - Place internal state assertions as close as possible to where they're required or used
  - Place external call assertions as close as possible to where the call happened
- "Highest" in the call stack:
  - Place authentication assertions as close as possible to the start of the externally accessible function
  - Place parameter assertions as close as possible to the start of the externally accessible function
- In places where an expensive state assertion will be repeated because a caller wants to branch based on the assertion (rather than allowing it to revert), add an "unsafe" version of the internal call that skips checks
Token Manager: disallow minting and vesting tokens to itself (#734)
This change adds more checks to disallow usage of the `mint()` and `assignVested()` actions for the Token Manager itself:

- `mint()`: you should use `issue()` instead (separate action protected by a separate role)
- `assignVested()`: Assigning vestings to the Token Manager makes the accounting awkward when calculating the spendable balance of the Token Manager itself. The Token Manager itself never has a spending limit (see `onTransfer()`, so the concept of a vesting does not work very well.

This change also modifies the way `_isBalanceIncreaseAllowed()` works, by always allowing increases to the Token Manager's balance.

Previously, using `assign()` with the Token Manager as the recipient would fail if its balance was already over the limit (see [previously failing test](https://github.com/aragon/aragon-apps/pull/734/files#diff-17c8771ab9c99065348cbba1b3fba658R179)). Although this isn't something to really worry about, it does show this check wasn't fully aligned with the balance assumptions (of it not applying to the Token Manager itself).
Finance: consistent payment creation (#711)
Splits up `newPayment()` into:

- `newScheduledPayment()`: _always_ saves a `RecurringPayment`, for repeating or scheduled future payments
- `newImmediatePayment()`: _only_ for single, immediately-payable transactions

It also modifies the execution behaviour of payments. Previously, it was possible to call `executePayment()` or `receiverExecutePayment()` even if no payments were made (only a single `PaymentFailure` event would be emitted). Now calling these two functions will cause a revert, with more obvious error messages (either you hadn't waited long enough, or there wasn't enough balance in the Vault).

We still allow `newScheduledPayment()` to create a "failing" payment, as I think it'd still be useful to schedule payments even if you can't immediately pay them out.
Finance: cosmetic clarifications for newImmediatePayment() (#749)
* Finance: fix newImmediatePayment()'s explaination of its interval param to reflect using MAX_UINT
* Finance: reorder newImmediatePayment() to be above newScheduledPayment()
Finance: rename RecurringPayments to ScheduledPayments (#735)
Now that the function name is called `newScheduledPayment()`, I thought it might make sense to rename the struct to also be `ScheduledPayment`.
Finance: rename maxRepeats to maxExecutions (#739)
Renames `repeats` to `executions`, `maxRepeats` to `maxExecutions`, and `paymentRepeats` to `paymentExecutions` to better reflect their parameters and states variables' meaning.

In particular, a `maxRepeats` of 1 is quite confusing as to whether it means you can repeat the payment once (e.g. actually execute it twice), or only execute it once.

@sohkai sohkai requested a review from facuspagnuolo Mar 28, 2019

sohkai added some commits Mar 28, 2019

@tintinweb

This comment has been minimized.

Copy link

tintinweb commented Apr 3, 2019

As a result of the change review we did not find any blocking issues. Please find a summary of our notes below.

  • comments directly related to the changes in this PR are discussed inline with the commit (see comment-inline tag below).
  • general observations not directly related to this change are provides as a comment

Commit

General Observations

  • Finance

    • (ℹ) (comment-inline) finance#L690 could be <= to state that end-time cannot be start time (logically impossible as period > 1days)
    • (ℹ) getMaxPeriodTransitions can be restricted to pure (const)
  • TokenManager

    • (ℹ) unused imports
      • uint256helpers
      • ERC20
  • Voting

    • Note - canforward passes the evmscript to canperform but it is never checked.
      Note - unnamed parameter in public method
      voting#L188-L196
  • Code Complexity

    • Note - The recent changes increased the overall complexity of the codebase (tradeoff gas/security; making sure all paths are properly checked).
  • Highlighting/Tagging unsafe functions

    • Note - Make sure to consistently mark unsafe functions that are not performing security checks and communicate how developers can quickly see whether functions are safe or not (e.g. add comments to function description)
  • Future Migration Note - Compatibility with solidity >= 0.5.0

Compiling your contracts...
===========================
Error: CompileError: ParsedContract.sol:120:105: ParserError: Expected ',' but got reserved keyword 'reference'
    event NewPayment(uint256 indexed paymentId, address indexed recipient, uint64 maxExecutions, string reference);
* @return False if the controller does not authorize the approval
*/
function onApprove(address, address, uint) public returns (bool) {
function onApprove(address, address, uint) public onlyToken returns (bool) {

This comment has been minimized.

Copy link
@tintinweb

tintinweb Apr 3, 2019

token callbacks (caller is minime) can be set external
onTransfer
onApprove
proxyPayment

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

See #770.

// Even though it is tested, solidity-coverage doesnt get it because
// MiniMeToken is not instrumented and entire tx is reverted
require(msg.sender == address(token), ERROR_PROXY_PAYMENT_WRONG_SENDER);
function proxyPayment(address) public payable onlyToken returns (bool) {

This comment has been minimized.

Copy link
@tintinweb

tintinweb Apr 3, 2019

consider throwing an exception in proxyPayment already instead of relying on the caller (minime-token) to throw if the method returns false (unless it is a defined interfaces intended to be used that way)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

Not quite sure if I understood the exception:

unless it is a defined interfaces intended to be used that way

We are relying on the MiniMe token to revert on this false return, but this can only be called from the token (and should only be when it receives ETH).

Throwing an error ourselves seems a bit redundant, and we more or less completely trust the MiniMe implementation in the Token Manager. I've added a small notice about this in #770, but this is something that should be noted in the app's README (🤞 I will move the outdated specs from our wiki to the repo in 2-3 weeks).

This comment has been minimized.

Copy link
@tintinweb

tintinweb Apr 10, 2019

👍 looks good.

// Be careful here to not overflow; if startTime + periodDuration overflows, we set endTime
// to MAX_UINT64 (let's assume that's the end of time for now).
uint64 endTime = _startTime + settings.periodDuration - 1;
if (endTime < _startTime) { // overflowed

This comment has been minimized.

Copy link
@tintinweb

tintinweb Apr 3, 2019

could be <= to state that end-time cannot be start time (logically impossible as period > 1days as per check init/setPeriodDuration)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

Not super sure about this change; the < check is the "safe math" check, so it might be less obvious as <=.

This comment has been minimized.

Copy link
@tintinweb

tintinweb Apr 10, 2019

agree, might be better to keep this as a strict overflow check.

@sohkai

This comment has been minimized.

Copy link
Member Author

sohkai commented Apr 5, 2019

Voting: mock time in tests rather than use timetravel (#737)

Token Manager: use TimeHelpers and mock time in tests (#738)

Agreed, we are making changes to fix how we use these mocks now 😄 : see #766.


Voting

Note - canforward passes the evmscript to canperform but it is never checked.
Note - unnamed parameter in public method voting#L188-L196

This is to conform with the IForwarder's canForward() interface. Technically we could pass a new, empty bytes array, but that'd cost a bit more.


Future Migration Note - Compatibility with solidity >= 0.5.0

Ouch :(. We'll make this change in the future, since it requires a frontend change to use the new variable name in the ABI.

@@ -19,7 +19,6 @@ import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";

contract TokenManager is ITokenController, IForwarder, AragonApp {
using SafeMath for uint256;
using Uint256Helpers for uint256;

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

See #771 for removing unused imports.

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

LGTM! Left some minor comments/questions

Show resolved Hide resolved apps/finance/contracts/Finance.sol Outdated
require(ERC20(_token).safeTransferFrom(msg.sender, this, _amount), ERROR_TOKEN_TRANSFER_FROM_REVERTED);
} else {
// Ensure that the ETH sent with the transaction equals the amount in the deposit
require(msg.value == _amount, ERROR_VALUE_MISMATCH);

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 8, 2019

Contributor

We are dropping this check for the case where _token == ETH, was it intentional?

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Author Member

Yes, this was a result of "bubbling" the parameter checks to the "top" of the stack.

It's now covered by deposit() directly, and in the fallback, no check is needed.

@@ -172,9 +176,9 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
require(_periodDuration >= 1 days, ERROR_INIT_PERIOD_TOO_SHORT);
settings.periodDuration = _periodDuration;

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 8, 2019

Contributor

I think we could extract the two lines above to an internal function like _setPeriodDuration or so and re-use it within setPeriodDuration as well.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Author Member

For sake of consistency with other require() checks, I've opted to keep this check duplicated, since it's a parameter check.

However, I've modified it so it uses a constant instead of a magic number in #781.

r[2] = _c;
r[3] = _d;
r[4] = _e;
r[5] = _f;

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 8, 2019

Contributor

Should we move this one to ACLSyntaxSugar?

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Author Member

I had debated this, but this is such a super specific instance that I don't think it'd make sense to add back in ACLSyntaxSugar. Apps that have giant params lists like this will need their own syntax sugar :).

cosmetic(Finance): update @dev wording to be consistent
Co-Authored-By: sohkai <qisheng.brett.sun@gmail.com>

sohkai added some commits Apr 10, 2019

Token Manager: small improvements for ITokenController interface (#770)
Changes the `ITokenController` methods to be `external`, as they never need to be called from within the Token Manager itself.
Finance: rename MAX_UINT to MAX_UINT256 (#772)
Makes these variables a little bit more clear and easy to search for.

@sohkai sohkai changed the title [DO NOT MERGE] Track @next contract changes (Aragon 0.7-part.2) Merge @next contract changes (Aragon 0.7-part.2) Apr 10, 2019

@sohkai sohkai changed the title Merge @next contract changes (Aragon 0.7-part.2) Track @next contract changes (Aragon 0.7-part.2) Apr 10, 2019

@sohkai sohkai merged commit 1d4756e into master Apr 10, 2019

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the next branch Apr 15, 2019

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