Sushi Trident contest phase 1 - t11s's results

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