First Flight #4: Boss Bridge
First Flight #4
Beginner FriendlyFoundryBridge
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

The version of ECDSA.recover used could lead to denial of service

happyformerlawyer

Summary

In BossBridge.sol, the sendToL1 function calls ECDSA.recover using the hashed message, v, r, s as parameters. ECDSA.recover using those parameters in turn calls ECDSA.tryRecover using the same parameters. The version of ECDSA.tryRecover called can lead to denial of service if s > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0 or if the signature uses positions 0/1 for v instead of 27/28.

This can be avoided by instead using a different version of ECDSA.recover (the one that takes bytes32 hash, bytes memory signature) that won't lead to this problem.

Vulnerability Details

This ECDSA.tryRecover function is the one that is ultimately called by BossBridge.sol (BossBridge.sol calls ECDSA.recover which in turn calls the following function as an overload). See the note in the middle of the function about situations where this function reverts.

function tryRecover(
bytes32 hash,
uint8 v,
bytes32 r,
bytes32 s
) internal pure returns (address, RecoverError, bytes32) {
// EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature
// unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
// the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. Most
// signatures from current libraries generate a unique signature with an s-value in the lower half order.
//
// If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value
// with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or
// vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
// these malleable signatures as well.
if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
return (address(0), RecoverError.InvalidSignatureS, s);
}
​
// If the signature is valid (and not malleable), return the signer address
address signer = ecrecover(hash, v, r, s);
if (signer == address(0)) {
return (address(0), RecoverError.InvalidSignature, bytes32(0));
}
​
return (signer, RecoverError.NoError, bytes32(0));
}

Impact

There could be a denial of service which is not necessary since you can instead just call a different recovery function in ECDSA.sol and avoid the DOS.

Tools Used

Manual review

Recommendations

Use the following ECDSA.recover function instead. This function calls a different ECDSA.tryRecover function which forces s to be small enough and v to be in the right position, so you won't ever have a denial of service for those reasons.

function recover(bytes32 hash, bytes memory signature) internal pure returns (address) {
(address recovered, RecoverError error, bytes32 errorArg) = tryRecover(hash, signature);
_throwError(error, errorArg);
return recovered;
}

Then also make the following changes to withdrawTokensToL1 and sendToL1 to use one signature instead of v, r, s

function withdrawTokensToL1(
address to,
uint256 amount,
bytes memory signature
) external {
uint256 timestamp = block.timestamp;
sendToL1(
signature,
timestamp,
(abi.encode(
address(token),
0, // value
abi.encodeCall(
IERC20.transferFrom,
(address(vault), to, amount)
)
)
);
}
function sendToL1(
bytes memory signature,
uint256 timestamp,
bytes memory message
) public nonReentrant whenNotPaused {
if(signature.length != 65) {
revert BossBridge__InvalidSignature();}
​
if(block.timestamp > timestamp + 5 minutes) {
revert BossBridge__SignatureExpired();}
​
address signer = ECDSA.recover(
MessageHashUtils.toEthSignedMessageHash(keccak256(message)),
signature
);
​
if (!signers[signer]) {
revert L1BossBridge__Unauthorized();
}
​
(address target, uint256 value, bytes memory data) = abi.decode(
message,
(address, uint256, bytes)
);
​
(bool success, ) = target.call{value: value}(data);
if (!success) {
revert L1BossBridge__CallFailed();
}
}
Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.