code423n4 commented 3 years ago

Handle

t11s

Vulnerability details

Impact

If the chain was to fork (or someone was to sign messages to use permit on a local fork), permit signatures made on either fork could be replayed on the other. This could allow users who permit to each other (or contracts that can be upgraded) to steal funds from each other.

Proof of Concept

The DOMAIN_SEPARATOR is set in the constructor, which causes this issue. If block.chainid changes the DOMAIN_SEPARATOR separator will not be updated.

https://github.com/sushiswap/trident/blob/9da0311c5a555fc16e5e18ca0a04fe5895365391/contracts/pool/TridentERC20.sol#L30-L37

This is advised against in the security considerations section of EIP2612: https://eips.ethereum.org/EIPS/eip-2612

"If the DOMAIN_SEPARATOR contains the chainId and is defined at contract deployment instead of reconstructed for every signature, there is a risk of possible replay attacks between chains in the event of a fututre chain split."

Tools Used

None.

Recommended Mitigation Steps

Either compute DOMAIN_SEPARATOR on the fly for each permit or cache a starting value in the constructor, and if chainid changes fall back to recomputing on the fly.

Here are some implementations of this:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/draft-EIP712.sol https://github.com/yieldprotocol/yield-utils-v2/blob/main/contracts/token/ERC20Permit.sol https://github.com/boringcrypto/BoringSolidity/blob/master/contracts/Domain.sol https://github.com/dapp-org/dappsys-v2/blob/a59e20576ef9febcd8ac350f8fad363e55c5277a/src/token.sol#L96-L98

maxsam4 commented 3 years ago

I knew this was coming xD

We fixed this in our codebase after your tweet. However, I agree with Dan on this. There are much bigger problems if the network forks than replaying. Therefore, this is an unrealistic scenario with limited impact.

I'd suggest changing the severity to 1.

alcueca commented 3 years ago

We could have a long discussion here, and I would prefer to stop reporting this vulnerability as it really is unrealistic. However, the sponsor confirmed and fixed it, so I can't dismiss it.

As a low-effort find, I'm downgrading it to 1 out of respect to real severity 2's found.