Tigris Trade contest - philogy's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L79 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Position.sol#L9 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/MetaContext.sol#L9

Vulnerability details

Impact

The use of EIP-2771 allows for meta transactions that authenticate users not only via msg.sender but also via "trusted" forwarders who are meant to authenticate the caller with some alternative mechanism e.g. message signatures. However the forwarders defined by the MetaContext parent contract can be arbitrarily assigned by the owner() address. This would allow a malicious owner to bypass caller authentication entirely by approving a custom forwarder, once in place the owner could then leverage ERC20 approvals, modify positions and withdraw assets of users without their express authentication.

Proof of Concept

As the Position and Trading contract owner():

1. Create and deploy a malicious forwarder, example: https://gist.github.com/Philogy/411c631885a237d29eec8ae3b5c6f013
2. Enable malicious forwarder on target contracts (Position, Trading) by calling inherited setTrustedForwarder method
3. Steal position by calling the Position's contract transferFrom or safeTransferFrom methods, imitating the user using the malicious forwarder.
4. Withdraw assets from the stolen position

Note that step 3. and 4. can be changed to not only steal positions but for instance leverage any remaining approval from a user to the Trading contract by calling Trading.addToPosition or Trading.addMargin on their behalf, increasing the potential damage even further.

Tools Used

Manual review.

Recommended Mitigation Steps

Either remove EIP2771 support entirely or ensure that there's a single, known forwarder which is hard coded as a constant or set upon deploy as an immutable variable to prevent the owner from being able to arbitrarily set custom forwarders.