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

Make ERC2771Context return original sender address if msg.data.length <= 20 #4481

Merged
merged 9 commits into from
Jul 25, 2023

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Jul 25, 2023

Fixes an audit finding. Report will be published soon.

The current implementation of the ERC2771Forwarder doesn't check that the msg.data has a length of at least the ERC2771 suffixed address, nullifying the result and producing the address(0) whenever accessing an out-of-bounds calldata location (i.e. calldata with size 19).

This PR checks for the correct length and falls back to the original sender.

Fixes LIB-984

@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2023

🦋 Changeset detected

Latest commit: f056ce5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw ernestognw requested review from frangio and Amxx July 25, 2023 20:35
Amxx
Amxx previously approved these changes Jul 25, 2023
@ernestognw
Copy link
Member Author

I thought accepting a suggestion doesn't dismiss reviews, but it does.
Can you reapprove @Amxx?

@ernestognw ernestognw requested a review from Amxx July 25, 2023 20:56
@ernestognw ernestognw requested a review from frangio July 25, 2023 21:31
@Amxx Amxx enabled auto-merge (squash) July 25, 2023 21:38
@Amxx Amxx merged commit 28d9ac2 into OpenZeppelin:master Jul 25, 2023
frangio pushed a commit that referenced this pull request Jul 25, 2023
@frangio
Copy link
Contributor

frangio commented Jul 26, 2023

For future observers, the title of this PR says <= 20 but it should have been < 20. The code does the latter (correctly).

Woodpile37 added a commit to Woodpile37/EIPs that referenced this pull request Oct 29, 2023
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to upgrade
@openzeppelin/contracts from 4.9.3 to 5.0.0.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

*Warning:* This is a major version upgrade, and may be a breaking
change.
- The recommended version is **4 versions** ahead of your current
version.
- The recommended version was released **21 days ago**, on 2023-10-05.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>@openzeppelin/contracts</b></summary>
    <ul>
      <li>
<b>5.0.0</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0">2023-10-05</a></br><a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0">
Read more </a>
      </li>
      <li>
<b>5.0.0-rc.2</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.2">2023-10-02</a></br><ul>
<li><code>AccessManager</code>: Make <code>schedule</code> and
<code>execute</code> more conservative when delay is 0.</li>
</ul>
      </li>
      <li>
<b>5.0.0-rc.1</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.1">2023-09-28</a></br><ul>
<li>Upgradeable Contracts: No longer transpile interfaces, libraries,
and stateless contracts. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4636"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4636/hovercard">ethereum#4636</a>)</li>
<li><code>AccessManager</code>, <code>AccessManaged</code>,
<code>GovernorTimelockAccess</code>: Ensure that calldata shorter than 4
bytes is not padded to 4 bytes. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4624"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4624/hovercard">ethereum#4624</a>)</li>
<li><code>AccessManager</code>: Use named return parameters in functions
that return multiple values. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4624"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4624/hovercard">ethereum#4624</a>)</li>
</ul>
      </li>
      <li>
<b>5.0.0-rc.0</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.0">2023-09-19</a></br><a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.0">
Read more </a>
      </li>
      <li>
<b>4.9.3</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.9.3">2023-07-28</a></br><div
class="markdown-alert markdown-alert-note"><p><span
class="color-fg-accent text-semibold d-inline-flex flex-items-center
mb-1"><svg class="octicon octicon-info mr-2" viewBox="0 0 16 16"
version="1.1" width="16" height="16" aria-hidden="true"><path d="M0 8a8
8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0
0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75
0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8
6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"></path></svg>Note</span><br>
This release contains a fix for <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-g4vp-m682-qqmp">GHSA-g4vp-m682-qqmp</a>.</p></div>
<ul>
<li><code>ERC2771Context</code>: Return the forwarder address whenever
the <code>msg.data</code> of a call originating from a trusted forwarder
is not long enough to contain the request signer address (i.e.
<code>msg.data.length</code> is less than 20 bytes), as specified by
ERC-2771. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4481"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4481/hovercard">ethereum#4481</a>)</li>
<li><code>ERC2771Context</code>: Prevent revert in
<code>_msgData()</code> when a call originating from a trusted forwarder
is not long enough to contain the request signer address (i.e.
<code>msg.data.length</code> is less than 20 bytes). Return the full
calldata in that case. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4484"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4484/hovercard">ethereum#4484</a>)</li>
</ul>
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases">@openzeppelin/contracts
GitHub release notes</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiJjMDUxYjcyNi0zNzViLTRjODgtYmI2NS1iOTJjYTk5MjgxOWQiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6ImMwNTFiNzI2LTM3NWItNGM4OC1iYjY1LWI5MmNhOTkyODE5ZCJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/woodpile37/project/ada51a90-dc7c-4239-82d9-c94c84ce1884?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/woodpile37/project/ada51a90-dc7c-4239-82d9-c94c84ce1884/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/woodpile37/project/ada51a90-dc7c-4239-82d9-c94c84ce1884/settings/integration?pkg&#x3D;@openzeppelin/contracts&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"c051b726-375b-4c88-bb65-b92ca992819d","prPublicId":"c051b726-375b-4c88-bb65-b92ca992819d","dependencies":[{"name":"@openzeppelin/contracts","from":"4.9.3","to":"5.0.0"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/woodpile37/project/ada51a90-dc7c-4239-82d9-c94c84ce1884?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"ada51a90-dc7c-4239-82d9-c94c84ce1884","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":4,"publishedDate":"2023-10-05T18:00:56.344Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":true,"isBreakingChange":true,"priorityScoreList":[]})
--->
Woodpile37 added a commit to Woodpile37/EIPs that referenced this pull request Oct 30, 2023
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to upgrade
@openzeppelin/contracts from 4.9.3 to 5.0.0.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

*Warning:* This is a major version upgrade, and may be a breaking
change.
- The recommended version is **4 versions** ahead of your current
version.
- The recommended version was released **24 days ago**, on 2023-10-05.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>@openzeppelin/contracts</b></summary>
    <ul>
      <li>
<b>5.0.0</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0">2023-10-05</a></br><a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0">
Read more </a>
      </li>
      <li>
<b>5.0.0-rc.2</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.2">2023-10-02</a></br><ul>
<li><code>AccessManager</code>: Make <code>schedule</code> and
<code>execute</code> more conservative when delay is 0.</li>
</ul>
      </li>
      <li>
<b>5.0.0-rc.1</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.1">2023-09-28</a></br><ul>
<li>Upgradeable Contracts: No longer transpile interfaces, libraries,
and stateless contracts. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4636"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4636/hovercard">ethereum#4636</a>)</li>
<li><code>AccessManager</code>, <code>AccessManaged</code>,
<code>GovernorTimelockAccess</code>: Ensure that calldata shorter than 4
bytes is not padded to 4 bytes. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4624"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4624/hovercard">ethereum#4624</a>)</li>
<li><code>AccessManager</code>: Use named return parameters in functions
that return multiple values. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4624"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4624/hovercard">ethereum#4624</a>)</li>
</ul>
      </li>
      <li>
<b>5.0.0-rc.0</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.0">2023-09-19</a></br><a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0-rc.0">
Read more </a>
      </li>
      <li>
<b>4.9.3</b> - <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.9.3">2023-07-28</a></br><div
class="markdown-alert markdown-alert-note"><p><span
class="color-fg-accent text-semibold d-inline-flex flex-items-center
mb-1"><svg class="octicon octicon-info mr-2" viewBox="0 0 16 16"
version="1.1" width="16" height="16" aria-hidden="true"><path d="M0 8a8
8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0
0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75
0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8
6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"></path></svg>Note</span><br>
This release contains a fix for <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-g4vp-m682-qqmp">GHSA-g4vp-m682-qqmp</a>.</p></div>
<ul>
<li><code>ERC2771Context</code>: Return the forwarder address whenever
the <code>msg.data</code> of a call originating from a trusted forwarder
is not long enough to contain the request signer address (i.e.
<code>msg.data.length</code> is less than 20 bytes), as specified by
ERC-2771. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4481"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4481/hovercard">ethereum#4481</a>)</li>
<li><code>ERC2771Context</code>: Prevent revert in
<code>_msgData()</code> when a call originating from a trusted forwarder
is not long enough to contain the request signer address (i.e.
<code>msg.data.length</code> is less than 20 bytes). Return the full
calldata in that case. (<a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/pull/4484"
data-hovercard-type="pull_request"
data-hovercard-url="/OpenZeppelin/openzeppelin-contracts/pull/4484/hovercard">ethereum#4484</a>)</li>
</ul>
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/OpenZeppelin/openzeppelin-contracts/releases">@openzeppelin/contracts
GitHub release notes</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiI2NGQ0NjcxOS01ZDIzLTQ1MjYtYWJiOC00OGEwZmQ1N2QxZjgiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjY0ZDQ2NzE5LTVkMjMtNDUyNi1hYmI4LTQ4YTBmZDU3ZDFmOCJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54/settings/integration?pkg&#x3D;@openzeppelin/contracts&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"64d46719-5d23-4526-abb8-48a0fd57d1f8","prPublicId":"64d46719-5d23-4526-abb8-48a0fd57d1f8","dependencies":[{"name":"@openzeppelin/contracts","from":"4.9.3","to":"5.0.0"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"f0dcf1c9-ecf1-445b-bc07-e8f73c595f54","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":4,"publishedDate":"2023-10-05T18:00:56.344Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":true,"isBreakingChange":true,"priorityScoreList":[]})
--->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants