-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor solver calls to directly from Atlas #225
Conversation
…three separate functions that are each called by escrow contract. This new design now has Atlas calling the solver contracts instead of the executionEnvironment calling the solver contracts, which should increase the safety. The math / bid verification accounting has also been simplified, and a new SolverTracker struct has been introduced to assist with data passing between the layers.
Some clean up changes:
|
…e of the solver try/catch
src/contracts/atlas/Escrow.sol
Outdated
} else if (errorSwitch == SolverOperationReverted.selector) { | ||
result = 1 << uint256(SolverOutcome.SolverOpReverted); | ||
} else if (errorSwitch == PostSolverFailed.selector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error and SolverOutcome names differ, suggest renaming the error:
- error SolverOperationReverted();
+ error SolverOpReverted();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here 1632ec2
src/contracts/atlas/Escrow.sol
Outdated
} else if (errorSwitch == SolverBidUnpaid.selector) { | ||
result = 1 << uint256(SolverOutcome.BidNotPaid); | ||
} else if (errorSwitch == BalanceNotReconciled.selector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error and SolverOutcome names differ, suggest renaming the error:
- error SolverBidUnpaid();
+ error BidNotPaid();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here 1632ec2
src/contracts/atlas/Escrow.sol
Outdated
} else if (errorSwitch == IntentUnfulfilled.selector) { | ||
result = 1 << uint256(SolverOutcome.IntentUnfulfilled); | ||
} else if (errorSwitch == SolverBidUnpaid.selector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntentUnfulfilled
does not seem to be thrown anywhere. Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed here 1632ec2
src/contracts/atlas/Escrow.sol
Outdated
} else if (errorSwitch == InvalidEntry.selector) { | ||
// DAppControl is attacking solver contract - treat as AlteredControl | ||
result = 1 << uint256(SolverOutcome.AlteredControl); | ||
} else if (errorSwitch == CallbackNotCalled.selector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CallbackNotCalled
does not seem to be thrown anywhere, do we still need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I've split up the checks at end of solverCall()
to revert with either BalanceNotReconciled or CallbackNotCalled. I think the granularity for diagnosing what exactly caused the failure is useful. See 1632ec2
@@ -85,6 +85,14 @@ contract Storage is AtlasEvents, AtlasErrors, AtlasConstants { | |||
/// @return calledBack Boolean indicating whether the solver has called back via `reconcile`. | |||
/// @return fulfilled Boolean indicating whether the solver's outstanding debt has been repaid via `reconcile`. | |||
function solverLockData() public view returns (address currentSolver, bool calledBack, bool fulfilled) { | |||
return _solverLockData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the point of this, does it do something? Seems like this wrapping will increase the gas usage by about 88 from my tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its more gas efficient to call an internal function than a public one, but we still need to expose that function publically for dapps or solvers to call. But good catch, the outer function should be external not public. Fixed here c1e3e1b
@@ -361,6 +330,7 @@ contract ExecutionEnvironmentTest is BaseTest { | |||
(status,) = address(executionEnvironment).call(postOpsData); | |||
} | |||
|
|||
/* | |||
function test_solverMetaTryCatch() public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't test_solverMetaTryCatch be removed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but we should also add 2 new tests, for solverPreTryCatch and solverPostTryCatch. I've added a note to refactor this one into those 2 and left it commented as some of this test code may be reused in the new ones. Will add it to the list of tests to do after the audit fixes.
Aside from those comments above, this all LGTM. From the issues above, they all seem to be addressed, except I'm not sure about https://github.com/spearbit-audits/review-fastlane/issues/149 That one feels like it's gonna need a thorough review. |
Co-authored-by: ephess <[email protected]>
…/atlas into atlas-calls-solver
endBalance = etherIsBidToken ? endBalance : address(this).balance; | ||
} | ||
// bidValue is not inverted; Higher bids are better; solver must deposit >= bidAmount | ||
if (!solverTracker.invertsBidValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the floor and ceiling records get taken after the pre and post solver calls for !invertsBidValue, but the records get taken beforehand for invertsBidValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. So to rephrase, if mode is invertsBidValue, the change is measured over preSolverCall, solverCall, and postSolverCall. But if the mode is normal bid value, the change is just measured over solverCall.
I think it could be because, in invertsBidValue mode, the assets must be transferred to the solver in the preSolver hook (because it would be less secure / more complex with approvals, to allow the solver to pull them in the solver call). So we take the starting balance measurement before a dapp might transfer assets in the preSolver hook. Same reasoning on the other side I think - assets representing the bid may be transferred to the solver in postSolver hook so check end balance after that. This way we support transferring those bid assets on either side of the solver call.
Whereas in normal bid mode, we just measure the assets the solver pays which only happens in solver call. So we can measure that type of bid more tightly (excluding changes due to pre and post solver hooks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you described would make sense, but that doesn't seem to be the implementation if i'm reading the code correctly. I worded it a bit weirdly at first. The below is the correct re-phrasing:
invertsBidValue the change is measured over preSolverCall and solverCall.
normal mode the change is measured over solverCall and postSolverCall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have a hook that takes place outside of the balance check for each option so that we can transform or otherwise adjust the balances. For the inverted bids, we wanted to be able to move inventory in so that we could then give it to the solver before the solver call, and for the normal bids we wanted to be able to move inventory out that we have received from the solver after the solver call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool makes sense, lets add comments to the code that document this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: This seems fine because the only party who can affect the bid amount calculation is the dapp, who in doing so knows they are affecting the bid this way. No other party can act between the solver execution and the end of the postSolverHook (end of bid phase calculation) other than the dapp.
Original: A consequence of this is that if a dapp calls contribute() in the postSolverHook in:
- normal mode this causes the contribution to be reduced from the bid amount calculation
- invertsBidBalue: this causes the contribution to be included in the bid amount calculation
Could this cause any issues?
This PR treats on-chain bid finding the same as regular known bids to determine solver payments. This seems intuitively right but Alex's comments seem to say we might want to do it some other way? https://github.com/spearbit-audits/review-fastlane/pull/1/files#r1561952304 |
I think Alex's comment there is more accurately implemented in the code after this PR - any ETH surplus in the EE (change measured across the pre and post solver hooks) is only considered to be the bid, and is completely separate from the solver's gas or borrowed ETH repayment. The gas / borrow repayment is handled by solver calling So I don't think there was any other way that was explored, but rather that the original idea has been implemented much more cleanly in this PR. |
(, calledback, success) = _solverLockData(); | ||
|
||
if (!calledback) revert CallbackNotCalled(); | ||
if (!success && deposits < claims + withdrawals) revert BalanceNotReconciled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesnt this check still trust this unreliable fulfilled flag? https://github.com/orgs/FastLane-Labs/projects/7/views/1?pane=issue&itemId=67398082
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it still uses the fulfilled flag - but the idea is that it should be reliable now. As of the Lock PR, borrow is limited to SolverOperations phase or before, and reconcile()
can only be called during the SolverOperations phase. So, if a solver calls reconcile()
and sets fulfilled = true
then we know it can't go back to unfulfilled as the dapp can no longer borrow - only repay further via contribute()
Audit issues resolved in this PR:
https://github.com/spearbit-audits/review-fastlane/issues/31
DAppControl.preSolverCall()
andDAppControl.postSolverCall()
to return nothing instead of a bool indicating success. Then, in EE, we only check thebool success
returned from the delegatecall to see if failed. No decoding of returneddata
needed. Fixed here 33f24c0https://github.com/spearbit-audits/review-fastlane/issues/103
https://github.com/spearbit-audits/review-fastlane/issues/110
_releaseSolverLock()
no longer called inside_getBidAmount()
.https://github.com/spearbit-audits/review-fastlane/issues/127
holdSolverLock()
completely. Call index is still incremented in_executeSolverOperation()
but should now be done once per solverOp, regardless of success or failure of the solverOp.https://github.com/spearbit-audits/review-fastlane/issues/138
validateBalances()
has been removed and instead we check that the solver has repaid anything owed via theirreconcile()
call by checking thefulfilled
bool from_solverLockData()
at the end ofsolverCall()
in Atlas. Note, we do a final check thatdeposits >= claims + withdrawals
so even if the solver didn't fully repay what they owe viareconcile()
, the postSolverCall hook has the opportunity to cover the remainder viacontribute()
.https://github.com/spearbit-audits/review-fastlane/issues/137
validateBalances()
case is resolved by removing the function and only including the necessary logic (no lock check needed) at the end ofatlasSolverCall()
.https://github.com/spearbit-audits/review-fastlane/issues/140
atlasSolverCall()
in SolverBase.sol checks that themsg.sender
is Atlas.https://github.com/spearbit-audits/review-fastlane/issues/161
https://github.com/spearbit-audits/review-fastlane/issues/195
_trySolverLock()
now reverts with anInsufficientEscrow
error which is caught and reflected inresult
.https://github.com/spearbit-audits/review-fastlane/issues/149
invertsBidValue
has been simplified a lot in this PR. This mode is when a solver's bid wins if its the lowest, and represents the amount of assets they will take instead of give, from the Execution Environment.Previously resolved audit issues affected by this PR:
https://github.com/spearbit-audits/review-fastlane/issues/150
Note: Atlas is currently about 800 bytes over the contract size limit. We will resolve the size limit issue after the major structural change PRs - most likely the upcoming locking mechanism PR and a gas refund accounting revamp PR.