raymogg
commented
3 years ago While the behaviour is undefined, the first if check for base == 0 will catch accounts that don't have any active positions in the market. Since all accounts are initialised in this state, this will remove any account that should not need to be settled. All other accounts, whether EOAs or contracts, can be settled if they have outstanding positions (base) in the market.
The settle function is meant to be able to be called by anyone on any account, so having it internal does to work around this either.
Will update our docs to reflect this, but disputing as its not really an issue at all.
cemozerr
Handle
0xRajeev
Vulnerability details
Impact
The public visible settle() function accepts an address parameter but there is no input validation to check if this is a valid address in the context of that Tracer market.
Impact: This function may be called with any address relevant to the market e.g. insuranceContract but not a user’s account. It is not clear what may happen in such cases and if it may lead to undefined behavior.
Proof of Concept
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualSwaps.sol#L427
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate any side-effects or undefined behavior if this is called with a non-user but market-relevant address such as insuranceContract. Enforce validation of only user accounts being accepted. Consider making this internal visibility. If not, document why these measures may not be necessary.