Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Add tx.origin to TEC approvals #1668

Merged
merged 8 commits into from
Mar 7, 2019

Conversation

abandeali1
Copy link
Member

@abandeali1 abandeali1 commented Mar 5, 2019

Description

  • Adds a txOrigin field to the Coordinator approval message. The transaction must be executed by this address. This prevent's a user's transaction from being front-runned when it is intended to be used in conjunction with external smart contract calls.
  • Optimizes transaction and approval hashing

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage decreased (-0.008%) to 85.134% when pulling 9471510 on feat/contracts/tec-txorigin-check into babe013 on development.

@abandeali1 abandeali1 force-pushed the feat/contracts/tec-txorigin-check branch from 80c7db2 to b1d86a7 Compare March 7, 2019 19:06
Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

A couple changes. Other than that - looks good!

let arrayContentsStart := add(addressArray, 32)

// Loop through array
for {let i:= 0} lt(i, arrayByteLen) {i := add(i, 32)} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should save some additional gas with this:

for {let i := arrayContentsStart} lt(i, arrayContentsEnd) {i := add(i, 32)} {
    let arrayElement := mload(i)
}

One less add per iteration.

let arrayContentsStart := add(addressArray, 32)

// Loop through array
for {let i:= 0} lt(i, arrayByteLen) {i := add(i, 32)} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

// Verify that Ethereum tx signer is the same as the approved txOrigin
require(
tx.origin == txOrigin,
"INVALID_SENDER"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be good to use INVALID_ORIGIN. Similar instances of INVALID_SENDER refer to an invalid msg.sender

./exchange-libs/contracts/src/LibExchangeErrors.sol:    string constant internal INVALID_SENDER = "INVALID_SENDER";                                  // Invalid `msg.sender`.

@abandeali1 abandeali1 merged commit 9e03e1c into development Mar 7, 2019
@abandeali1 abandeali1 deleted the feat/contracts/tec-txorigin-check branch March 13, 2019 20:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants