Closed code423n4 closed 1 year ago
gzeon-c4 marked the issue as primary issue
gzeon-c4 marked the issue as duplicate of #535
gzeon-c4 marked the issue as not a duplicate
gzeon-c4 marked the issue as duplicate of #489
livingrockrises marked the issue as disagree with severity
gas price is signed by the owner of smart account and is capped by tx.gasPrice in case of ether payment. I agree if startGas can be manipulated this end up being more gas refund to the relayer.
livingrockrises marked the issue as sponsor confirmed
signature + "0".repeat(482000)
^ when i make this change in the test case, I am getting below
Error: overflow [ See: https://links.ethers.org/v5-errors-NUMERIC_FAULT-overflow ] (fault="overflow", operation="toNumber", value="40724240000000000", code=NUMERIC_FAULT, version=bignumber/5.7.0)
probably because it exceeds block gas limit
gzeon-c4 marked the issue as satisfactory
Hey @gzeon-c4 @livingrockrises!
This is a good finding.
However, it is not complete. As mentioned in issue #489 user funds can be stolen by adding any number of zero bytes to the msg.data
, while this report states that only the signature
elongation leads to the attack. The mitigation step does not solve the issue though.
need to remove dependency on msg.data.length all together
Hey @gzeon-c4 @livingrockrises!
This is a good finding.
However, it is not complete. As mentioned in issue #489 user funds can be stolen by adding any number of zero bytes to the
msg.data
, while this report states that only thesignature
elongation leads to the attack. The mitigation step does not solve the issue though.
True, that's why I have the other issue as primary. The attack can be even more efficient by padding in an internal call so you don't even need to pay for the top level calldata cost.
Lines of code
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192-L219
Vulnerability details
Impact
Relayers can take signed transactions and append zeroes to the signature parameter to artificially increase the gas cost and startGas estimation. This causes additional cost for the signer and increases the relayers reimbursement. The cost/reimbursement increases linearly with gas price.
Proof of Concept
To calculate the refund payment SmartAccount uses a heuristic to estimate the startGas (startGas = gasleft() + 21000 + msg.data.length * 8;). Later it proceeds to subtract the remaining gas from this initial estimation. By appending zeroes to the signature parameter, thus increasing the msg.data.length, the heuristic will overestimate the startGas, which in turn increases the reimbursement.
Hardhat example
To see the impact of this manipulation add the following to test\smart-wallet\testGroup1.ts. I've chosen a relatively high gas price of 100 gwei and the most simple transaction of sending ether to show maximum impact of this finding. The loss scales linearly with gas price, but will typically be much less than shown here, because it assumes one transaction to fill an entire ethereum block. Though it should be feasible for relayers to submit such single transaction bundles to extract this value. On other blockchains with higher block size, the loss can also be increased.
The signer pays 20 times the amount he signed for, while the relayer takes a small, but not insignificant profit!
Tools Used
VS code, hardhat
Recommended Mitigation Steps
Check the signature length at the beginning of execTransaction. E.g.
require(signatures.length == 65, "Invalid signature length");