code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L59 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L75

Vulnerability details

Impact

solmate ERC20 permit does not check s for mallebiltiy, which means it allows anyone to modify the signature of an existing signature in a specific way without access to the private key and replay the transaction with a new valid signature.

Proof of Concept

the following code will create a new valid signature:

bytes32 constant public groupOrder = 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141;
bytes32 s1 = bytes32(uint256(groupOrder)-uint256(s));
uint8 v1 = v==27 ? 28 : 27;

Tools Used

Recommended Mitigation Steps

Need to add the following checks before calling asset.permit

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
  revert("SignatureValidator#recoverSigner: invalid signature 's' value");
}

if (v != 27 && v != 28) {
  revert("SignatureValidator#recoverSigner: invalid signature 'v' value");
}

FortisFortuna commented 2 years ago

Can you demonstrate a test script where someone can steal sfrxETH?

FortisFortuna commented 2 years ago

Apparently we are using ECDSA recover, so it is probably invalid

0xean commented 2 years ago

downgrading to QA