Skip to content

Commit

Permalink
Fix Revert message propagation (#163)
Browse files Browse the repository at this point in the history
This PR fixes a revert message propagation issue where the length of
revert bytes was the **offset** of the return data in memory and not its
actual length 🤦. It turns out that because of how ABI encoding works,
the tests were not catching this issue since in that particular test the
offset was alway larger than the actual length of the revert message. In
theory, this is UB (as the offset it not guaranteed to be larger than
the message, although, in practice it is not a serious issue, as it
appears the compiler is allocating memory for the raw `call` result and
then **copying with a loop** the ABI-decoded `returnData` memory bytes
after it, so the offset of `returnData` will always be greater than its
size, and the current implementation just returns too many bytes).

Adjusted tests to ensure the exact revert data is propagated.
  • Loading branch information
nlordell authored Nov 22, 2023
1 parent 406ef30 commit 307a3b4
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 5 deletions.
2 changes: 1 addition & 1 deletion 4337/contracts/Safe4337Module.sol
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
if (!success) {
// solhint-disable-next-line no-inline-assembly
assembly ("memory-safe") {
revert(add(returnData, 0x20), returnData)
revert(add(returnData, 0x20), mload(returnData))
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions 4337/test/eip4337/EIP4337ModuleExisting.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,9 @@ describe('Safe4337Module - Existing Safe', () => {
const transaction = await entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001')).then((tx) => tx.wait())
const logs = transaction.logs.map((log) => entryPoint.interface.parseLog(log)) ?? []
const emittedRevert = logs.find((l) => l?.name === 'UserOpReverted')
const [decodedError] = ethers.AbiCoder.defaultAbiCoder().decode(['string'], `0x${emittedRevert?.args.reason.slice(10) || ''}`)
expect(decodedError).to.equal('You called a function that always reverts')
expect(emittedRevert?.args.reason).to.equal(
reverterContract.interface.encodeErrorResult('Error', ['You called a function that always reverts']),
)
})
})
})
5 changes: 3 additions & 2 deletions 4337/test/eip4337/EIP4337ModuleNew.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,9 @@ describe('Safe4337Module - Newly deployed safe', () => {
const receipt = await transaction.wait()
const logs = receipt.logs.map((log) => entryPoint.interface.parseLog(log))
const emittedRevert = logs.find((log) => log?.name === 'UserOpReverted')
const [decodedError] = ethers.AbiCoder.defaultAbiCoder().decode(['string'], `0x${emittedRevert?.args.reason.slice(10) ?? ''}`)
expect(decodedError).to.equal('You called a function that always reverts')
expect(emittedRevert?.args.reason).to.equal(
reverterContract.interface.encodeErrorResult('Error', ['You called a function that always reverts']),
)
expect(await safe.isDeployed()).to.be.true
})
})
Expand Down

0 comments on commit 307a3b4

Please sign in to comment.