werg
commented
3 years ago This has merit: particularly the part about self-constructed pairs. We either need much more rigorous checks or a a process for vetting & approving pairs. The latter is likely more gas efficient.
Email address
mail@gpersoon.com
Handle
gpersoon
Eth address
gpersoon.eth
Vulnerability details
The functions crossSwapTokensForExactTokens and crossSwapExactTokensForTokens of MarginRouter.sol do not check who is calling the function. They also do not check the contents of pairs and tokens They also do not check if the size of pairs and tokens is the same
registerTradeAndBorrow within registerTrade does seem to do an entry check (require(isMarginTrader(msg.sender)...) however as this is an external function msg.sender is the address of MarginRouter.sol, which will verify ok.
Impact
Calling these functions allow the caller to trade on behalf of marginswap, which could result in losing funds. It's possible to construct all parameters to circumvent the checks. Also the "pairs" can be fully specified; they are contract addresses that are called from getAmountsIn / getAmountsOut and from pair.swap. This way you can call arbitrary (self constructed) code, which can reentrantly call the marginswap code.
Proof of concept
Based on source code review. A real attack requires the deployed code to be able to construct the right values.
Tools used
remix
Recommended mitigation steps
Limit who can call the functions Perhaps whitelist contents of pairs and tokens Check the size of pairs and tokens is the same