code-423n4 / 2021-06-tracer-findings

1 stars 0 forks source link

orders and orderToSig mappings #47

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

pauliax

Vulnerability details

Impact

Contract Trader has 2 mappings: orders and orderToSig. I see that orderToSig also stores Perpetuals.Order inside it, so I wonder if it really was necessary to separate these mappings as some state (order) is duplicated among them. It may be a bit more efficient to access orders without signatures but it also makes it more error-prone as you need to keep the invariant that orders match in these mappings. Currently I don't see an exact problem as orderToSig are only set in function grabOrder and never used in code anywhere but I am not sure if it is really necessary.

Tracer representetive's answer on Discord: 'Yeah thats a good point on the Trader mapping, one does look redundant now as they both store the order itself. I think originally one was mutated and one wasn't, but then that functionality got moved into the filled mapping anyway. Seems safe to remove orders and simply reference the orderToSig mapping'.

Recommended Mitigation Steps

Remove orders and simply reference the orderToSig mapping.

raymogg commented 3 years ago

Confirmed, however probably not a low risk as there is no actual errors present from the code. More of an optimisation and preventive measure, making it most likely a 0.

cemozerr commented 3 years ago

Marking this as non-critical as it doesn't seem to pose any risks.

loudoguno commented 3 years ago

changed risk from 1 to 0 as per judges sheet