Additional commitments #40
Conversation
0age
left a comment
0age
left a comment
There was a problem hiding this comment.
two possible issues and a micro-optimization, take a look but otherwise LGTM for the POC!
|
|
||
| // Confirm the allocation - calculate balance differences | ||
| for { let i := 0 } lt(i, permittedLength) { i := add(i, 1) } { | ||
| let commitmentMemLoc := mload(add(commitmentsContent, mul(i, 0x20))) // load the absolute pointer to the Lock struct |
There was a problem hiding this comment.
micro-optimization: instead of incrementing by one and then multiplying i by 0x20 every time in the loop, you can derive permittedLengthWords by doing one shl 5 and then iterate by 0x20 and just use i directly
| diffBalance := add(diffBalance, additionalCommitmentAmount) | ||
|
|
||
| // Set the containsAdditionalCommitments flag if the additional commitment amount is not zero | ||
| containsAdditionalCommitments := or(containsAdditionalCommitments, gt(additionalCommitmentAmount, 0)) |
There was a problem hiding this comment.
i believe iszero(iszero(additionalCommitmentAmount)) is a tad cheaper / simpler
| mstore(add(previousBalancesPointer, mul(i, 0x20)), oldBalance) | ||
|
|
||
| // Add the additional commitment amount. | ||
| diffBalance := add(diffBalance, additionalCommitmentAmount) |
There was a problem hiding this comment.
could you overflow this by providing a massive additional commitment amount?
There was a problem hiding this comment.
Good question. My initial thought is, that it would not really matter, since it would only just worst case shrink the diffBalance? And then you would just allocate less tokens then you deposited I guess? Additionally, the claim hash needs to add up in the end...
Will need to think about this more though, especially since the HybridAllocator and the OnChainAllocator handle the actual allocation part differently.
There was a problem hiding this comment.
Additionally (at least in the case of the hybrid allocator), the off chain signer of the allocator would have needed to sign for this:
For the on chain allocator, we either revert here due to the overflow, or due to the fact that the previous balance was not sufficient:
There was a problem hiding this comment.
So while this might not affect our current implementations, it does seems like a good check to improve the security on this library itself to minimize unexpected behavior!
| AL.prepareAllocation(nonce88, recipient, idsAndAmounts, arbiter, expires, typehash, witness, ALLOCATOR_ID); | ||
| if (context.length > 0) { | ||
| // Potential off chain nonce provided | ||
| HybridAllocationContext calldata allocationContext = _decodeContext(context); |
There was a problem hiding this comment.
not sure whether or not this is going to work; do we have any tests of this functionality yet? the decode-bytes-to-calldata logic is always a little gnarly/counterintuitive
There was a problem hiding this comment.
Thanks for looking so detailed at this!
Had all the tests and some tiny fixes committed, but forgot to push this.
I believe this is working as intended, I came up with a similar design for the ERC7683 order data parsing.
This is the setup and the test for this:
sc-allocators/test/HybridAllocator.t.sol
Line 2074 in e7bd57f
sc-allocators/test/HybridAllocator.t.sol
Line 2479 in e7bd57f
I don't have any tests that check the revert scenarios on this _decodeContext function yet though.
| mstore(add(previousBalancesPointer, mul(i, 0x20)), oldBalance) | ||
|
|
||
| // Add the additional commitment amount to the difference in balance. This amount must be verifiably unallocated. | ||
| diffBalance := add(diffBalance, additionalCommitmentAmount) |
There was a problem hiding this comment.
is it possible to overflow this value by providing a massive additionalCommitmentAmount?
| let previousBalancesPointer := add(previousBalances, 0x20) | ||
|
|
||
| // Confirm the allocation - calculate balance differences | ||
| for { let i := 0 } lt(i, permittedLength) { i := add(i, 1) } { |
There was a problem hiding this comment.
micro-optimization: for this and some other low-level iterators, we could modify the length once via a mul 0x20 (or shl 5) and then increment i by 0x20 to save all the internal muls
Pull Request
Description
Allows to allocate additional amounts that are not part of the deposit in the
prepare/execute allocationflow, as well as in thepermit2Allocationflow.For the OnChainAllocator, the contract itself will ensure these amounts are unallocated at the time of the call.
For the HybridAllocator, you will have to provide a signature from the off chain signers that confirms these additional amounts are still unallocated. During the prepare and execute flow, this additionally requires the off chain signer to provide a valid nonce that can be used for this, which is required to make the claimHash predictable.
We can be sure of the users intentions for this additional allocation, since the claimHash must be registered on chain.