Adding support for V1 of the compact #8
Conversation
ccashwell
left a comment
ccashwell
left a comment
There was a problem hiding this comment.
Overall, this is a fantastic implementation. I added a few small notes/questions inline, but no showstoppers really 👍
src/allocators/ERC7683Allocator.sol
Outdated
| bytes calldata allocatorData // Arbitrary data provided by the arbiter. | ||
| ) external override onlyCompact returns (bytes4) { | ||
| uint256 length = idsAndAmounts.length; | ||
| if (length > 1) { |
There was a problem hiding this comment.
Why reject batch claims? What actually stops us from supporting here?
There was a problem hiding this comment.
Updated the ERC7683Allocator to be based on the new OnChainAllocator contract, which works with batchCompacts:
https://github.com/Uniswap/sc-allocators/blob/OnChainAllocator/src/allocators/ERC7683Allocator.sol
| function addSigner(address signer_) external onlySigner { | ||
| signers[signer_] = true; | ||
| signerCount++; | ||
| } |
There was a problem hiding this comment.
In order to avoid a potential vulnerability in scenarios where ecrecover returns address(0) during signature validation, we should explicitly block the zero address here:
| function addSigner(address signer_) external onlySigner { | |
| signers[signer_] = true; | |
| signerCount++; | |
| } | |
| function addSigner(address signer_) external onlySigner { | |
| require(signer_ != address(0), InvalidSigner()); | |
| signers[signer_] = true; | |
| signerCount++; | |
| } |
| function replaceSigner(address newSigner_) external onlySigner { | ||
| signers[msg.sender] = false; | ||
| signers[newSigner_] = true; | ||
| } |
There was a problem hiding this comment.
| function replaceSigner(address newSigner_) external onlySigner { | |
| signers[msg.sender] = false; | |
| signers[newSigner_] = true; | |
| } | |
| function replaceSigner(address newSigner_) external onlySigner { | |
| require(newSigner_ != address(0), InvalidSigner()); | |
| signers[msg.sender] = false; | |
| signers[newSigner_] = true; | |
| } |
mgretzke
force-pushed
the
the-compact-v1
branch
from
71d5ae5 to
110bd3c
Compare
mgretzke
force-pushed
the
the-compact-v1
branch
from
110bd3c to
dc40666
Compare
| import {ITheCompact} from '@uniswap/the-compact/interfaces/ITheCompact.sol'; | ||
| import {IHybridAllocator} from 'src/interfaces/IHybridAllocator.sol'; | ||
|
|
||
| contract HybridAllocator is IHybridAllocator { |
There was a problem hiding this comment.
nit: top level natspec explaining the high level purpose and functinoality
src/allocators/HybridAllocator.sol
Outdated
| } | ||
|
|
||
| // Check if the signer is an authorized allocator address | ||
| return signers[ecrecover(digest, v, r, s)]; |
There was a problem hiding this comment.
for safety its often worth a special check and revert if ecrecover returns address(0) => ie invalid signature
There was a problem hiding this comment.
Is that required, even if we ensure that signers[address(0)] can never be true?
added support for IOnChainAllocator
On chain allocator
Clean up codebase
…into new-mandate
New mandate
Pull Request
Description
Adding the HybridAllocator and the HybridERC7683 allocator.
HybridAllocator
HybridERC7683
Not all contracts are tested yet.