16 de dezembro de 2021
Introdução
A 1inch equipe nos pediu para revisar e auditar seus Protocolo de Ordem Limitada contratos inteligentes v2. Analisamos o código e agora publicamos nossos resultados.
Objetivo
Auditamos o commit 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
da 1inch/limit-order-protocol
repositório. Em escopo estavam os seguintes contratos:
- OrderMixin.sol
- OrderRFQMixin.sol
- PredicateHelper.sol
- RevertReasonParser.sol
- Permitable.sol
- ChainlinkCalculator.sol
- ArgumentsDecoder.sol
- AmountCalculator.sol
- NonceManager.sol
- LimitOrderProtocol.sol
- ImmutableOwner.sol
- InteractiveNotificationReceiver.sol
- AggregatorInterface.sol
- IDaiLikePermit.sol
Todos os outros arquivos e diretórios de projetos (incluindo testes), juntamente com dependências e projetos externos, teoria dos jogos e design de incentivos, também foram excluídos do escopo desta auditoria. Supôs-se que o código externo e as dependências contratuais funcionassem conforme documentado e os serviços de back-end fornecidos pela 1inch agiam no melhor interesse do protocolo.
Saúde geral
Em geral, achamos a base de código do projeto legível e bem organizada, embora pudesse se beneficiar de uma documentação mais extensa, especialmente em torno dos blocos de código assembly, casos extremos do protocolo, ativos/predicados/recursos externos que serão usados, responsabilidades/limitações do serviço back-end fornecido e interações entre os atores. O projeto não mede esforços para tornar as ações eficientes em termos de gás, ocasionalmente até mesmo correndo o risco de tornar o código mais difícil de raciocinar; levantamos questões relacionadas a isso abaixo. Durante toda a auditoria, a equipe da 1inch esteve altamente disponível, ágil e muito fácil de trabalhar.
Visão geral do sistema
O Protocolo de Ordem Limitada permite a ordem makers
para assinar ordens fora da cadeia para trocas de tokens. O protocolo facilita então o preenchimento de pedidos previamente assinados por pedido takers
. Os pedidos são altamente extensíveis e podem chamar contratos externos estáticamente em vários pontos do processo de atendimento do pedido. Essa extensibilidade confere utilidade ao protocolo, mas adiciona complexidade e uma maior superfície de ataque para os próprios pedidos.
É importante observar que não há armazenamento on-chain dos detalhes do pedido. O estado de preenchimento ou cancelamento dos pedidos só é rastreado por meio de hashes de pedidos. Isso exige que os pedidos sejam compartilhados ponto a ponto ou por meio de uma parte centralizada. Neste caso, a equipe 1inch pretende atuar como parte centralizada, agregando ordens assinadas e utilizando essas ordens como fonte de liquidez para seus demais protocolos. Os pedidos serão publicados por meio de API própria para que os usuários possam interagir com eles.
Essa centralização dá à equipe da 1inch extremo controle sobre quais pedidos são publicados e, por fim, executados. Isto também lhes dá a capacidade de censurar ordens, o que pode ser útil no caso de ordens maliciosas ou enganosas, mas também pode ser abusado e permitir-lhes atacar qualquer outro utilizador no caso de uma ordem favorável, não mostrando-a através da API.
Funções privilegiadas
Embora os contratos em que a função é utilizada estivessem fora do escopo, uma função privilegiada foi identificada. Um immutableOwner
é definido para o criador de um contrato de procuração no momento da construção e é usado para limitar o acesso ao proxy external
funções.
Dependências externas e suposições de confiança
O design deste protocolo necessita de componentes fora e dentro da cadeia, e este modelo híbrido pode ser usado para mitigar alguns vetores de ataque que identificamos em nosso relatório, mas o custo dessa capacidade é o aumento da dependência da equipe e da infraestrutura de 1 polegada.
Além disso, o Limit Order Protocol fornece funções destinadas a recuperar preços dos oráculos Chainlink. Presumimos que esses oráculos eram honestos, acessíveis e funcionavam adequadamente.
Além disso, devido à flexibilidade de uma encomenda, existem vários pontos de contacto com contratos externos que não são validados. Isso significa que um usuário mal-intencionado pode abusar de tais chamadas e personificar predicados, ativos ou oráculos com contratos maliciosos para executar ações durante o atendimento de pedidos. Embora o projeto esteja protegido em algumas áreas contra reentrada, tais vetores podem causar ataques de negação de serviço ou ordens de spam não detectadas. A equipe da 1inch está ciente de que certos problemas podem surgir ao usar contratos desconhecidos para o protocolo e indicou sua intenção de que apenas os principais ativos “blue chip” sejam totalmente suportados pelo projeto. No entanto, deve-se notar que mesmo com os ativos mais populares existem comportamentos intrínsecos de cada ativo que podem causar problemas em protocolos que não os abordam adequadamente, como cobrar uma taxa durante as transferências com USDT ou retornar um código de erro em vez de um sucesso booleano com cTokens.
Descobertas
Aqui apresentamos nossas descobertas.
Gravidade crítica
Nenhum.
Alta gravidade
[H01] Dados inconsistentes passados para _makeCall
No OrderMixin
contrato, o _makeCall
função é usada para transferir ativos do tomador ao criador e depois do criador ao tomador. Nesta última transferência, o _makeCall
função é passada incorretamente no pedido makerAsset
como último parâmetro, quando deveria ser o do pedido makerAssetData
.
Como resultado, qualquer funcionalidade de proxy que dependa do makerAssetData
argumento será quebrado.
Para ser consistente com o apelo anterior à _makeCall
e para oferecer suporte total à funcionalidade de proxy, considere atualizar o order.makerAsset
parâmetro para order.makerAssetData
.
Update: Fixo em pull request # 57.
[H02] Pedidos privados parcialmente atendidos podem ser atendidos por qualquer pessoa
O protocolo permite a criação de ordens privadas e públicas. Em encomendas privadas, apenas o allowedSender
endereço, especificado pelo fabricante durante a criação do pedido, é capaz de atender o pedido.
No entanto, no OrderMixin
contrato, validação para o allowedSender
endereço tem escopo incorreto, o que significa que só é avaliado dentro da lógica que trata do primeiro preenchimento de um pedido. Se um pedido privado for parcialmente atendido, o cheque do allowedSender
o endereço não está mais acessível e o pedido pode ser preenchido por qualquer pessoa.
Para esclarecer a intenção sobre se algum usuário deve ser capaz de atender pedidos privados parcialmente preenchidos ou não, considere documentar o motivo do comportamento atual ou validar o allowedSender
endereço fora do escopo do primeiro preenchimento para garantir que ele será validado sempre que um preenchimento for tentado.
Update: Fixo em pull request # 58.
[H03] Fabricante malicioso pode aproveitar preenchimentos parciais para roubar ativos do tomador
Encomendas do OrderMixin
contrato tem a possibilidade de ser parcialmente preenchido. Para suportar preenchimentos parciais, o protocolo requer uma forma de calcular ambos os lados das trocas. Ambos getMakerAmount
e getTakerAmount
os campos são definidos pelo autor do pedido exatamente para esse fim.
Ao preencher um pedido, os compradores devem fornecer o makingAmount
ou de takingAmount
valores, bem como um thresholdAmount
valor. Existem dois caminhos de código diferentes que podem ser seguidos, com base se o makingAmount
ou de takingAmount
foi fornecido.
A primeira é quando o makingAmount
parâmetro é definido. Poderia truncar que o makingAmount
valor e também Calcule o takingAmount
valor por isso. Nesta situação, o thresholdAmount
garante que o takingAmount
o valor obtido é não inesperadamente grande.
A segunda é quando o takingAmount
parâmetro é definido. Nesse caso, será Calcule o makingAmount
valor, com possibilidade de truncando-o e recalculando o takingAmount
valor se isso acontecer. Nesta situação, o thresholdAmount
valor garante que o makingAmount
o valor retornado é não inesperadamente pequeno.
Existem dois métodos de exploração, cada um exclusivo para um dos caminhos de código mencionados anteriormente. Esses métodos de exploração exigem getMakerAmount
e getTakerAmount
funções. Uma simples implementação destas funções teria um comportamento idêntico ao AmountCalculator
's getMakerAmount
e getTakerAmount
funções, mas com uma opção codificada que os forçará a retornar um valor controlado pelo invasor quando necessário.
O primeiro padrão de exploração menos grave envolve o primeiro caminho de código onde o makingAmount
o valor é especificado em uma ordem de preenchimento. Um fabricante mal-intencionado esperaria por uma ordem de preenchimento que especificasse makingAmount
para aparecer no mempool para poder executá-lo. Eles drenariam todo o valor, exceto 1, do lado do fabricante e então forçariam _callGetTakerAmount
para devolver o valor especificado na conta do usuário thresholdAmount
valor (ou seu subsídio se for menor). Quando a transação do usuário finalmente for concluída, ele trocará seu valor total thresholdAmount
valor de takerAsset
para uma única unidade de makerAsset
. Esta exploração é limitada pela quantidade dada pelo thresholdAmount
valor ou o montante do takerAsset
o usuário permitiu no LimitOrderProtocol
contrato.
O segundo padrão de exploração mais grave envolve o segundo caminho de código onde o takingAmount
o valor é especificado. O criador malicioso também esperaria por uma ordem de preenchimento que especificasse um takingAmount
valor para aparecer no mempool. Eles iriam liderar a transação e forçar o makingAmount
valor retornado por _callGetMakerAmount
função seja superior a ambos os remainingMakerAmount
e os votos de thresholdAmount
. Eles também definiriam o takingAmount
valor retornado por _callGetTakerAmount
ser a quantidade de takerAsset
ativo permitido no LimitOrderProtocol
pelo tomador. Quando a transação do tomador for concluída, ela será truncar o makingAmount
valor e depois recalcular o takingAmount
valor. No entanto, não é garantido que este recálculo seja inferior e, neste caso, esgotará o tomador de todos os takerAsset
que eles permitiram no contrato. Neste caminho de código, o thresholdAmount
valor é garantindo que o makingAmount
não é muito baixo, então pegando todos os compradores takerAsset
ativo está desmarcado. Os fundos perdidos são limitados pelo montante do takerAsset
ativo que o usuário permitiu no LimitOrderProtocol
contrato.
Estas explorações não são possíveis sem ordens parciais e, mais especificamente, ordens parciais com ações maliciosas. getMakerAmount
e getTakerAmount
implementações.
A questão principal do thresholdAmount
A verificação de valor é que ela cobre apenas um lado da troca, mas o outro lado pode ser manipulado por meio de frontrunning. Não há garantias de que o valor proposto originalmente pelo tomador permaneça inalterado. Considere remover makingAmount
truncamento de ambos os caminhos de código e reversão se o pedido não puder suportar um preenchimento tão grande quanto solicitado. Ao fazer isso, o thresholdAmount
pode ser usado para restringir suficientemente o outro lado da troca e evitar comportamentos inesperados, mesmo em ordens maliciosas.
Update: Fixo em pull request # 83.
Gravidade média
[M01] Argumentos estáticos passados após argumentos dinâmicos
No OrderMixin
contrato, o getTakerAmount
e getMakerAmount
campos de bytes são usados como argumentos para o _callGetTakerAmount
e _callGetMakerAmount
funções. Essas chamadas fornecem uma maneira de calcular um lado do swap com base no outro lado e permitem que os usuários atendam parcialmente os pedidos.
A getTakerAmount
/getMakerAmount
campos são variáveis dinâmicas e são compactados na frente do takerAmount
e makerAmount
valores no _callGetTakerAmount
e _callGetMakerAmount
funções. É possível que um fabricante mal-intencionado forneça mais dados do que o esperado no getTakerAmount
egetMakerAmount
campos para empurrar o takerAmount
e makerAmount
bytes além de onde eles devem estar ao serem decodificados na próxima função. Isso permite que o criador desloque a quantidade de tomador ou criador passado em bytes completos para a direita e até mesmo os substitua completamente se 32 bytes extras de dados forem fornecidos.
Os usuários já precisam revisar manualmente o getTakerAmount
e getMakerAmount
campos na ordem, mas essa técnica é bastante difícil de detectar. Também vale a pena notar que este ataque se aplica até mesmo a usuários internamente confiáveis getMakerAmount
e getTakerAmount
funções. Para a maioria dos ataques, fornecer um limite razoável evitará a perda de fundos.
Para evitar isso, considere codificar os argumentos estáticos antes dos argumentos dinâmicos para evitar fornecer aos argumentos dinâmicos um método para controlar os argumentos estáticos.
Update: Não consertado. A equipe de 1 polegada declarou:
Teremos cuidado extra com a validação de getters. Tentaremos implementar a validação de sanidade de getters em nosso SDK que ajudará na filtragem de pedidos potencialmente maliciosos.
[M02] Pedidos ERC721 podem ser manipulados
É possível trocar mais do que apenas ERC20s através do OrderMixin
implantando um contrato que compartilha o mesmo seletor de função do IERC20 transferFrom
, e fornecendo esse contrato como o makerAsset
ou de takerAsset
em uma ordem.
Os proxies fora do escopo, ou seja, ERC721Proxy
, ERC721ProxySafe
e ERC1155Proxy
contratos seguem esse padrão para fornecer suporte para ERC721
e ERC1155
fichas. Como os proxies devem ser chamados com o mesmo padrão de um IERC20 transferFrom
chamada, a assinatura deve começar com address from
, address to
e uint256 amount
. Qualquer outra coisa que os proxies exijam pode ser passada depois e é definida na ordem como makerAssetData
e takerAssetData
.
ERC1155s podem transferir naturalmente vários tokens de identificação iguais de uma só vez, o que significa que o ERC1155Proxy
contrato faz uso do amount
campo. Por outro lado, ERC721
s não têm um uso óbvio para o amount
campo. Como representam tokens não fungíveis, um tokenId específico terá apenas um existente, tornando o amount
campo inútil. Por causa disso, a implementação para ambos ERC721Proxy
e ERC721ProxySafe
contratos usam o necessário amount
campo como o tokenId
ao invés.
Essa sobrecarga do amount
parâmetro cria a possibilidade de preenchimento parcial ERC721
pedidos para comprar tokens listados separadamente a preços com desconto. Por exemplo, pode haver um caso em que um único usuário tenha vários ERC721
s do mesmo contrato cuja transferência é permitida pelo ERC721Proxy
contrato e os lista em ordens de limite separadas.
Se as ordens limitadas também fornecerem o getMakerAmount
e getTakerAmount
campos, será possível preencher parcialmente estes ERC721
ordens. Desde que a ordem amount
campo realmente corresponde ao tokenId
, um usuário mal-intencionado pode preencher parcialmente o ERC721
com o tokenId mais alto, resultando em um makingAmount
/takingAmount
de uma ERC721
que poderia corresponder a um valor inferior tokenId
. O resultado é o ERC721
com o mais baixo tokenId
seria transferido ao preço de (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Esta exploração tem alguns requisitos:
- Múltiplo
ERC721
s do mesmo contrato sejam permitidos em qualquerERC721
procuração por um único proprietário. - Pedido aberto para um dos
ERC721
isso não é o mais baixotokenId
dos permitidos. - Preenchimentos parciais permitidos no pedido.
Para eliminar completamente a possibilidade de parcial ERC721
preenchimentos, considere separar os amount
e tokenId
argumentos. Quer os argumentos estejam separados ou não, considere também documentar isso para alertar os usuários sobre esse comportamento e evitar esse padrão no futuro.
Update: Fixo em pull request # 59.
[M03] Suposições decimais não documentadas
A LimitOrderProtocol
contrato herda o ChainlinkCalculator
contrato através do OrderMixin
contrato. Este contrato expõe duas funções para permitir o uso de oráculos Chainlink durante o verificação de predicados e a pesquisa do quantidade do fabricante/valor do tomador.
No entanto, o contrato faz suposições não documentadas sobre o número de casas decimais que os oráculos Chainlink devem reportar, bem como o número de casas decimais que os parâmetros da função devem conter. Em determinados cenários, isto pode levar a comportamentos inesperados, incluindo a avaliação incorreta dos ativos e a perda involuntária de fundos.
Mais especificamente, ao longo do contrato a suposição implícita é que os oráculos Chainlink reportarão com 18 casas decimais de precisão. No entanto, não todos os oráculos Chainlink relatório com esse número de casas decimais. Na verdade, se o oráculo relatar um par de tokens em termos de moeda (USD, por exemplo), ele terá apenas 8 casas decimais de precisão. Como não há restrições qual oráculos podem ser usados, não devem ser feitas suposições implícitas sobre o número de casas decimais com as quais eles reportarão.
Da mesma forma, há uma suposição implícita de que o amount
parâmetro para o ChainlinkCalculator
funções usarão 18 casas decimais, juntamente com a declaração explícita enganosa de que o singlePrice
função Calculates price of token relative to ETH scaled by 1e18
. Na realidade, mesmo com um oráculo que parece relatório com 18 casas decimais, o valor de retorno do singlePrice
função seria escalonada pelo número de casas decimais do amount
parâmetro, que pode não necessariamente ter 18 casas decimais.
Da mesma forma, o doublePrice
A função assume que dois oráculos Chainlink reportarão com o mesmo número de casas decimais, fazendo com que o resultado da função se desvie das expectativas.
Considere documentar explicitamente as suposições sobre o número de decimais que os parâmetros e valores de retorno devem ter. Além disso, considere limitar os cálculos que dependem de oráculos que quebram essas suposições ou fazer com que os cálculos relevantes levem em consideração o número real de casas decimais.
Update: Fixo em pull request # 75.
Baixa gravidade
[L01] Constantes não declaradas explicitamente
Existem algumas ocorrências de valores literais sendo usados com significado inexplicável na base de código. Por exemplo:
- No
OrderMixin
contrato, o_remaining
o mapeamento está semanticamente sobrecarregado (conforme explicado no problema Sobrecarga semântica de mapeamento) para rastrear a quantidade de ativo restante para um pedido parcialmente preenchido assim como se um pedido foi completamente atendido. Especificamente,0
significa que nenhum preenchimento associado a um pedido foi feito,1
significa que um pedido não pode mais ser atendido e qualquer coisa maior que1
significa que há um valor restante associado ao pedido que pode ser potencialmente atendido. - No
ChainlinkCalculator
contrato, o valor literal1e18
é usado nosinglePrice
função.
Para melhorar a legibilidade do código e facilitar a refatoração, considere definir uma constante para cada número mágico, dando-lhe um nome claro e autoexplicativo. Para valores complexos, considere adicionar um comentário embutido explicando como foram calculados ou por que foram escolhidos.
Update: Fixo em pull request # 75 e pull request # 76.
[L02] Partes maliciosas podem impedir a execução de ordens permitidas
A OrderMixin
contrato permite que usuários criadores enviem ordens permitidas portanto, eles podem ser executados em uma transação, em vez de precisar de uma transação separada para aprovações. Além disso, os recebedores de pedidos podem enviar sua própria licença durante o preenchimento da encomenda para o mesmo fim.
No entanto, como a licença do fabricante está contida dentro do ordem, tanto as licenças do fabricante quanto as do tomador estarão acessíveis enquanto a transação de atendimento do pedido estiver no mempool. Isso tornaria possível que qualquer usuário mal-intencionado obtivesse essas licenças e as executasse nos respectivos contratos de ativos enquanto executava a transação de preenchimento. Como essas licenças têm um nonce
para evitar um ataque de gastos duplos, a transação de preenchimento do pedido falharia como resultado da tentativa de usar a mesma permissão que acabou de ser usada durante o frontrun.
Embora não haja risco de segurança e o fabricante possa criar um novo pedido e pré-aprovar a transação, esse ataque certamente poderá impactar a usabilidade dos pedidos permitidos. Na verdade, um invasor motivado poderia bloquear todos os ordens permitidas com este ataque. Considere validar se a licença já foi enviada, ou se o subsídio é suficiente, durante o preenchimento do pedido. Considere também informar os usuários sobre esse possível ataque durante a composição do pedido.
Update: Não consertado. A equipe de 1 polegada afirma:
Já tivemos verificações de aprovação antes, mas decidimos simplificar o fluxo de licenças para reverter apenas em caso de aprovações malsucedidas. Pensaremos em maneiras de notificar os fabricantes sobre o problema.
[L03] Código duplicado
Existem instâncias de código duplicado na base de código. A duplicação de código pode causar problemas posteriormente no ciclo de vida de desenvolvimento e deixar o projeto mais sujeito à introdução de erros. Esses erros podem ser introduzidos inadvertidamente quando as alterações de funcionalidade não são replicadas em todas as instâncias de código que deveriam ser idênticas. Exemplos de código duplicado incluem:
Em vez de duplicar o código, considere ter apenas um contrato ou biblioteca contendo o código duplicado e usá-lo sempre que a funcionalidade duplicada for necessária.
Update: Parcialmente fixado em pull request # 60.
[L04] Conjunto de testes incorreto ou enganoso
Há casos no conjunto de testes em que os testes se desviam do comportamento esperado. Por exemplo:
- A
ChainlinkCalculator
contrato é herdado peloOrderMixin
contrato. Porém, durante os testesAmountCalculator.arbitraryStaticCall
função é usada para chamar oChainlinkCalculator
contrato como um contrato externo e independente. Mesmo que o resultado seja o esperado, o teste deve refletir o comportamento com o design atual do sistema e o caso de uso previsto, chamandoChainlinkCalculator
funciona diretamente sem usar a chamada estática arbitrária. - Embora os contratos de proxy estivessem fora do escopo, notamos que ao testar o protocolo com ativos ERC721, o
ERC721Proxy
contrato não é usado para trocar os ativos em seu suíte de teste.
Como o conjunto de testes em si está fora do escopo desta auditoria, considere revisar minuciosamente o conjunto de testes para garantir que todos os testes sejam executados com êxito de acordo com as especificações do protocolo.
Update: Fixo em pull request # 57, pull request # 59 e pull request # 61.
[L05] Erros e omissões em eventos
Em toda a base de código, os eventos geralmente são emitidos quando alterações confidenciais são feitas nos contratos. No entanto, muitos eventos carecem de parâmetros indexados e/ou faltam parâmetros importantes. Por exemplo:
Existem também ações sensíveis que carecem de eventos, como:
Considere indexar de forma mais completa os eventos existentes e adicionar novos parâmetros onde eles estiverem faltando. Além disso, considere emitir todos os eventos de forma tão completa que possam ser usados para reconstruir o estado do contrato por serviços fora da cadeia.
Update: Não consertado. No entanto, a equipe 1inch adicionou um orderRemaining
parâmetro para o OrderCanceled
evento em pull request # 62.
A equipe de 1 polegada afirma:
Descobrimos que apenas um subconjunto limitado de dados é necessário para satisfazer as necessidades do frontend. No caso de análise extensa, todos os campos sugeridos ficam disponíveis via rastreamento. Para
OrderRFQMixin
esperamos que os formadores de mercado criem sua própria maneira sofisticada de rastrear quais pedidos foram cancelados.
[L06] Mudanças no armazenamento durante a emissão do evento
No NonceManager
contrato, quando o NonceIncreased
evento é emitido, o nonce do remetente da mensagem também é aumentado.
A execução simultânea de várias operações pode tornar a base de código mais difícil de raciocinar, mais propensa a erros e pode fazer com que as operações sejam ignoradas ou mal compreendidas.
Para melhorar a intencionalidade geral, a legibilidade e a clareza do código, considere aumentar o valor nonce antes de emitir o evento.
Update: Fixo em pull request # 63.
[L07] Metodologias de decodificação inconsistentes podem causar discrepâncias nos resultados
Para suportar toda a sua extensibilidade e flexibilidade, o Protocolo de Ordem Limitada tem que lidar rotineiramente com dados de bytes dinâmicos e valores de retorno arbitrários de contratos externos. Como resultado, o protocolo inclui uma ArgumentsDecoder
biblioteca para converter com mais eficiência valores de bytes dinâmicos em tipos de dados básicos. No entanto, esta biblioteca não é usada exclusivamente e, em alguns casos, abi.decode
é usado em seu lugar. Além disso, alguns contratos estão usando abi coder v1
enquanto outros estão usando abi coder v2
. O primeiro tem um desempenho mais semelhante ao ArgumentsDecoder
biblioteca, enquanto a última realiza verificações adicionais durante a decodificação.
O uso inconsistente dessas diferentes metodologias de decodificação pode resultar em discrepâncias sutis entre a intenção e o comportamento real da base de código.
Por exemplo, a simulateCalls
função usa apenas o ArgumentsDecoder.decodeBool
função. Se o simulateCalls
é usada para verificar chamadas que seriam feitas na parte predicada de uma ordem, então seus resultados poderiam divergir do que realmente ocorre quando as condições predicadas são avaliadas, porque diferentes metodologias de decodificação são empregadas.
Assim, por exemplo, se um predicado faz um efeito externo staticcall
para alguma função que retorna um uint256
valor maior que um em vez do esperado bool
, então essa chamada será revertida, porque o valor de retorno é decodificado com abi coder v2
's abi.decode
que não aceitará valores como bool
. No entanto, se exatamente a mesma chamada for feita com simulateCalls
, então isso será simplesmente marcado como true
, Porque decodeBool
trata qualquer valor maior que zero como true
.
Para tornar o simulateCalls
função espelhar totalmente o comportamento das chamadas de predicado reais, considere modificá-la para usar abi.decode
.
Update: Fixo em pull request # 82.
[L08] Falta de validação de entrada
A fillOrderToWithPermit
e fillOrderTo
funções do OrderMixin
contrato, bem como o fillOrderRFQToWithPermit
e fillOrderRFQTo
funções do OrderRFQMixin
contrato, não valide o target
parâmetro de endereço.
Isso permite que um usuário passe inadvertidamente o endereço zero e, como resultado, bloqueie os ativos que deveria receber após atender um pedido.
Para garantir que os usuários não bloqueiem acidentalmente seus fundos, considere validar se o target
endereço não é igual ao endereço zero nas funções citadas.
Update: Fixo em pull request # 78.
[L09] Baixa cobertura de testes unitários
A cobertura de testes unitários para todo o projeto é de cerca de 75%, com alguns dos contratos tendo cobertura particularmente baixa.
Considerando a importância dos testes unitários para validar o código e evitar regressões ao refatorar e desenvolver novos recursos, incentivamos o aumento significativo da cobertura dos testes unitários para pelo menos 95% e a inclusão de casos extremos que cubram até mesmo situações improváveis.
Update: Não consertado.
[L10] Documentação in-line enganosa ou incompleta
Em toda a base de código, alguns casos de documentação inline enganosa e/ou incompleta foram identificados e devem ser corrigidos.
A seguir estão exemplos de documentação in-line enganosa:
- No
ChainlinkCalculator
contrato, osinglePrice
funções NatSpec@notice
etiqueta diz que issoCalculates price of token relative to ETH scaled by 1e18
, mas na verdade, seu resultado é o valor ofamount
tokens dimensionados por1e18
, onde o oráculo não pode reportar em termos de ETH (para um par que não inclua ETH, por exemplo). - No
OrderRFQMixin
contrato, oinvalidatorForOrderRFQ
funções NatSpec@return
etiqueta é enganoso, pois a cotação pode não ter sido preenchida para que o respectivo bit invalidador tenha sido definido. O pedido também pode ter sido cancelado. - Nas linhas 147, 165 e 188 of
OrderMixin.sol
, o NatSpec@param
tags não são gramaticais. - na linha 20 of
ERC1155Proxy.sol
,@notice
tag afirma que o hash calculado é o resultado do hash dofunc_733NCGU
função, onde deveria estar ofunc_301JL5R
em vez disso.
A seguir estão exemplos de documentação in-line incompleta:
- Funções no
AmountCalculator
contrato não descreve nenhum dos parâmetros. - No
ChainlinkCalculator
contrato, osinglePrice
edoublePrice
funções não descrevem todos os parâmetros. - No
ImmutableOwner
contrato, a variável pública e o modificador não têm NatSpec. - No
InteractiveNotificationReceiver
contrato, onotifyFillOrder
A função não descreve nenhum dos parâmetros. - No
LimitOrderProtocol
contrato, oDOMAIN_SEPARATOR
função não tem NatSpec. - Eventos e mapeamentos no
NonceManager
não tem NatSpec. - No
OrderRFQMixin
contrato,cancelOrderRFQ*
funções não descrevem os valores de retorno. - No
OrderMixin
contrato, várias funções carecem de NatSpec completo. - na linha 168 of
OrderMixin.sol
e on-line 71 ofOrderRFQMixin.sol
, está faltando o@dev
tag. - Funções no
PredicateHelper
contrato não descreve todos os parâmetros.
A documentação in-line clara é fundamental para delinear as intenções do código. As incompatibilidades entre a documentação in-line e a implementação podem levar a sérios equívocos sobre como se espera que o sistema se comporte. Considere corrigir esses erros para evitar confusão para desenvolvedores, usuários e auditores.
Update: Parcialmente corrigido. Documentação enganosa abordada em pull request # 75 e pull request # 77.
A equipe de 1 polegada afirma:
Corrigimos documentos enganosos. A conclusão da documentação será feita posteriormente.
[L11] Ordens DoS possíveis ao usar ganchos
A OrderMixin
O contrato implementa funcionalidade para atender pedidos genéricos de swap fora da cadeia que podem ter condições para seu sucesso. Durante o preenchimento do pedido, o pedido pode verifique as condições “predicadas” predefinidas antes de continuar com a execução.
No entanto, como essas condições predicadas podem ter como alvo a lógica de qualquer contrato arbitrário, um criador mal-intencionado pode induzir os compradores a acreditarem que uma ordem se comporta corretamente e que é válida ao verificá-la fora da cadeia, mas falhando ao tentar preencher a mesma ordem. na cadeia. Essa mudança no comportamento do predicado poderia ser feita executando algum estado variável do qual os predicados dependem, examinando o gás enviado ou mesmo quais endereços estão envolvidos na chamada, ou por alguma outra lógica.
Além disso, se o fabricante definiu um interação durante a troca, interactionTarget
O contrato poderia reverter-se ou revogar a permissão para impedir o atendimento bem-sucedido do pedido, levando essencialmente ao mesmo resultado que os predicados maliciosos.
Embora os ativos não estejam em risco, os usuários ou bots que encontrarem um pedido favorável terão o fardo maior de tentar identificar esses tipos de pedidos de spam que podem parecer legítimos à primeira vista. Caso não consigam identificar este tipo de encomendas, incorrerão em custos de gás desperdiçados. Para reduzir a quantidade de pedidos de spam, considere restringir os alvos disponíveis para esses ganchos. Considere também alertar os usuários sobre essa possibilidade antes de tentarem atender aos pedidos.
Update: Não consertado. A equipe de 1 polegada afirma:
Cuidamos disso em nosso back-end e pensaremos em maneiras de notificar possíveis compradores sobre o problema.
[L12] O arredondamento pode ser desfavorável para taker
No OrderMixin
e OrderRFQMixin
contratos, quando um pedido está sendo atendido e o tomador fornece apenas uma makingAmount
or takingAmount
valor, o protocolo tenta calcular o valor de contrapartida do swap.
Há dois problemas com esses cálculos, o primeiro é que não há documentação ou lógica limitando o número de casas decimais que os parâmetros de valor devem usar, o que abordamos no Suposições decimais não documentadas questão.
A segunda questão é que, no decorrer desses cálculos, o protocolo gira a favor do fabricante. A questão do arredondamento pode ser bastante agravada quando as premissas decimais implícitas são quebradas, mas mesmo quando tudo está nos termos esperados, o arredondamento ocorrerá com valores pequenos e ímpares.
Considere permitir que o tomador especifique uma quantidade mínima de makerAsset
activo que estão dispostos a receber juntamente com um montante máximo de takerAsset
ativo que estão dispostos a trocar, para que a aceitação de qualquer arredondamento seja mais explícita.
Update: Não consertado. A equipe de 1 polegada afirma:
O valor limite deve ser suficiente para a proteção do tomador.
[L13] Tratamento de pedidos contraditórios quando faltam parâmetros
No OrderMixin
contrato, o fillOrderTo
função faz chamadas internas para o _callGetMakerAmount
e _callGetTakerAmount
funciona sempre que um preenchimento é tentado e o makingAmount
ou de takingAmount
parâmetros são zero, respectivamente, ou se o makingAmount
valor é maior que o remainingMakerAmount
valor.
A _callGetMakerAmount
e _callGetTakerAmount
chamadas levarão a reversões se o pedido não tiver sido criado com o getMakerAmount
or getTakerAmount
parâmetros, respectivamente, e um preenchimento parcial está sendo executado.
An comentário embutido ao lado _callGetMakerAmount
e um comentário embutido ao lado _callGetTakerAmount
afirmam que “somente preenchimentos inteiros são permitidos” se o pedido não foi criado com getMakerAmount
or getTakerAmount
parâmetros.
Entretanto, existem caminhos de código para os quais isso não se aplica, porque esses caminhos não verificam o length
s de ambos getMakerAmount
e getTakerAmount
parâmetros.
Especificamente, quando um taker
especifica um takerAmount
valor para um pedido que possui apenas um getMakerAmount
, a menos que essa chamada para getMakerAmount
retorna um valor maior que remainingMakerAmount
, um preenchimento parcial pode ser executado em contradição com a documentação embutida.
Isso não deixa clara a intencionalidade desses caminhos de código. Se este for o comportamento esperado, considere modificar a documentação embutida para que seja mais explícita. Se este for um comportamento não intencional, considere sempre verificar os comprimentos de ambos os getMakerAmount
e os votos de getTakerAmount
parâmetros simultaneamente para que a implementação reforce o comportamento descrito pela documentação embutida.
Update: Fixo em pull request # 79.
[L14] Usando chamadas Chainlink obsoletas
A ChainlinkCalculator
O contrato destina-se a ser usado para consultar oráculos Chainlink. Isso é feito por meio de chamadas para seus latestTimestamp
e latestAnswer
métodos, ambos foram descontinuados. Na verdade, os métodos não estão mais presentes na API dos agregadores Chainlink a partir da versão três.
Para evitar possíveis incompatibilidades futuras com oráculos Chainlink, considere usar o latestRoundData
método em vez disso.
Update: Fixo em pull request # 67.
Notas e informações adicionais
[N01] Não importando interfaces
A AggregatorInterface
interface parece ser um subconjunto de código copiado de ChainLink
repositório de código público. A interface completa está incluída em ChainLink
pacote npm do contrato.
Quando possível, para diminuir o potencial de incompatibilidades de interface e problemas resultantes, em vez de redefinir e/ou reescrever as interfaces de outro projeto, considere usar interfaces instaladas através de seus pacotes npm oficiais.
Update: Fixo em pull request # 66.
[N02] Dependências de projeto obsoletas
Durante a instalação do dependências do projeto, o NPM avisa que um dos pacotes instalados, Highlight
, “não terá mais suporte nem receberá atualizações de segurança no futuro”.
Mesmo que seja improvável que este pacote possa causar um risco de segurança, considere atualizar a dependência que utiliza este pacote para uma versão mantida.
Update: Fixo em pull request # 64.
[N03] Chamadas externas para visualizar métodos não são chamadas estáticas
Durante a maior parte da base de código, o protocolo faz chamadas externas explicitamente por meio do OpenZeppelin functionStaticCall
método para restringir a possibilidade de mudanças de estado quando elas não são esperadas ou não desejáveis. No entanto, no ChainlinkCalculator
contrato, apesar da intenção de fazer ligações externas apenas para view
métodos em oráculos Chainlink, as chamadas externas no singlePrice
e doublePrice
funções não são feitas via explícita staticcall
s.
Embora não tenhamos identificado nenhuma preocupação imediata de segurança decorrente disso, para reduzir a superfície de ataque, melhorar a consistência e esclarecer a intenção, considere usar staticcall
s, para todas as chamadas externas para view
funções no ChainlinkCalculator
contrato.
Update: Não consertado. A equipe de 1 polegada afirma:
Achamos que a complicação da sintaxe anula as melhorias na consistência.
[N04] Não falhar antecipadamente com pedidos inválidos
No OrderMixin
contrato, o fillOrderTo
função lida com a condição especial quando um pedido não foi enviado anteriormente (remainingMakerAmount == 0
), mas não trata explicitamente a condição quando o pedido não é mais válido (remainingMakerAmount == 1
).
Neste último cenário, a função acabará por ser revertida, mas somente após a queima de quantidades não triviais de gás. Para esclarecer a intenção, aumentar a legibilidade e reduzir o uso de gás, considere lidar explicitamente com o cenário de pedido inválido no início da função.
Update: Fixo em pull request # 68.
[N05] Contratos auxiliares não marcados como abstratos
No Solidity, a palavra-chave abstract
é usado para contratos que não são contratos funcionais por direito próprio ou que não se destinam a ser usados como tal. Em vez de, abstract
os contratos são herdados por outros contratos no sistema para criar contratos funcionais independentes.
Em toda a base de código, há exemplos de contratos auxiliares que não são marcados como abstratos, apesar de não serem destinados a serem implantados por conta própria. Por exemplo, o AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
e PredicateHelper
os contratos são todos compostos por um conjunto básico de funções que se destinam a ser usadas por contratos herdados.
Considere marcar contratos auxiliares como abstract
para indicar claramente que eles foram projetados exclusivamente para adicionar funcionalidade aos contratos que os herdam.
Update: Não consertado. A equipe de 1 polegada afirma:
Esses ajudantes podem ser implantados separadamente. Eles são herdados apenas para economia de gás.
[N06] Ordenação inconsistente de funções
Em toda a base de código, a ordem das funções geralmente segue o ordem recomendada no Solidity Style Guide, qual é: constructor
, fallback
, external
, public
, internal
, private
.
Contudo, no OrderMixin
contrato, o public
checkPredicate
função se desvia do guia de estilo, dividindo o external
funções.
Para melhorar a legibilidade geral do projeto, considere padronizar a ordem das funções em toda a base de código, conforme recomendado pelo Solidity Style Guide.
Update: Fixo em pull request # 69.
[N07] Fluxo de atendimento de pedidos inconsistente
A OrderMixin
e RFQOrderMixin
ambos os contratos lidam com o atendimento de pedidos assinados, mas o fluxo geral de pedidos entre os dois contratos é inconsistente.
OrderMixin
's fillOrderTo
A função segue este fluxo geral (pseudocódigo):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
enquanto que RFQOrderMixin
é análogo fillOrderRFQTo
A função segue este fluxo (pseudocódigo):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
Não há insights na documentação sobre por que a primeira condicional em cada uma dessas duas funções difere, ou por que takingAmount
e makingAmount
ambos não podem ser zero na última função. Além disso, o caso em que tanto um makingAmount
e de um takingAmount
são fornecidos é muito mais fácil de raciocinar no fillOrderRFQTo
função, uma vez que é tratada claramente no final else
bloquear.
Para esclarecer a intenção e aumentar a legibilidade geral do código, considere padronizar o fluxo geral de pedidos entre esses dois contratos ou documentar explicitamente por que existem diferenças.
Update: Não consertado. A equipe de 1 polegada afirma:
Isso se deve às funções de precificação personalizadas em pedidos com limite. Desde
getMakerAmount
pode potencialmente diferir substancialmente degetTakerAmount
, pensamos que é melhor não tornar a opção padrão para o tomador, pois isso provavelmente irá confundi-los nos casos em que esses getters serão diferentes.
[N08] As mensagens de erro são formatadas de forma inconsistente ou enganosas
Em toda a base de código, o require
e revert
mensagens de erro, destinadas a notificar os usuários sobre as condições específicas que causam a falha de uma transação, foram encontradas formatadas de forma inconsistente.
Por exemplo, cada uma das mensagens de erro nas linhas 85 de OrderMixin.sol
, 16 de ERC721ProxySafe.sol
e 26 de Permitable.sol
empregar um estilo diferente.
Além disso, algumas mensagens de erro são enganosas:
As mensagens de erro têm como objetivo notificar os usuários sobre condições de falha, portanto, devem fornecer informações suficientes para que as correções apropriadas possam ser feitas para interagir com o sistema. Mensagens de erro não informativas prejudicam muito a experiência geral do usuário, diminuindo assim a qualidade do sistema. Além disso, mensagens de erro formatadas de forma inconsistente podem causar confusão desnecessária. Portanto, considere revisar toda a base de código para garantir que cada require
e revert
tem uma mensagem de erro formatada de forma consistente, precisa, informativa e fácil de usar.
Update: Parcialmente fixado em pull request # 81.
[N09] Uso inconsistente de variáveis de retorno nomeadas
Há um uso inconsistente de variáveis de retorno nomeadas no OrderMixin
contrato. Algumas funções retornar variáveis nomeadas, outras retornar valores explícitos, e outros declare uma variável de retorno nomeada, mas substitua-a com uma declaração de retorno explícita.
Considere adotar uma abordagem consistente para retornar valores em toda a base de código, removendo todas as variáveis de retorno nomeadas, declarando-as explicitamente como variáveis locais e adicionando as instruções de retorno necessárias quando apropriado. Isso melhoraria a explicitação e a legibilidade do código e também poderia ajudar a reduzir regressões durante futuras refatorações de código.
Update: Fixo em pull request # 73.
[N10] O cálculo de hash do pedido não está aberto à API
A external
funções remaining
, remainingRaw
e remainingsRaw
todos esperam um hash de pedido para uma operação bem-sucedida.
No entanto, a função auxiliar _hash
, que retorna o hash de um pedido, tem private
visibilidade. Isso significa que os usuários terão que empacotar manualmente partes dos pedidos e strings de domínio para obter o hash de um pedido.
Para evitar possíveis erros ao calcular hashes de pedidos e fornecer aos usuários um método para gerar o respectivo hash de um pedido, considere estender a visibilidade do _hash
função para public
e refatorando o nome para hash
para ser consistente com o resto do código.
Update: Fixo em pull request # 74.
[N11] Sobrecarga semântica de mapeamento
A _remaining
mapeamento no OrderMixin
O contrato é semanticamente sobrecarregado para rastrear o status dos pedidos e a quantidade restante de ativos disponíveis para esses pedidos.
Os três estados que ele pode assumir são:
0
: O hash do pedido ainda não foi visto.1
: O pedido foi cancelado ou totalmente atendido.- Qualquer
uint
maior que1
: O restantemakerAmount
disponível para ser preenchido no pedido mais1
.
Essa sobrecarga semântica requer empacotamento e desempacotamento desse valor durante lookup
, cancellation
, initialization
e storage
.
A sobrecarga semântica e a lógica necessária para habilitá-la podem estar sujeitas a erros e tornar a base de código mais difícil de entender e raciocinar. Também pode abrir a porta para regressões em atualizações futuras do código.
Para melhorar a legibilidade do código, considere rastrear o estado de conclusão dos pedidos em um mapeamento separado.
Update: Não consertado. A equipe da 1inch citou que uma correção aumentaria os custos do gás para o fillOrder
função.
[N12] Ordens com permissão permitem chamadas para contratos arbitrários
A OrderMixin
contrato herda o Permitable
contrato para permitir o preenchimento de ordens de transação única com ativos que aceitam tal permit
apelos à modificação dos subsídios.
No entanto, a chamadas para o Permitable
contract não validam se o alvo é um ativo permitido nem se é mesmo um ativo, o que poderia permitir que um usuário mal-intencionado passasse o endereço de um contrato arbitrário que poderia executar outra chamada antes que o preenchimento do pedido fosse concluído.
Embora o contrato seja protegido contra reentrada, é sempre recomendável reduzir a superfície de ataque e evitar a chamada de contratos externos durante a execução. Considere restringir o ativo envolvido na licença aos ativos envolvidos no pedido ou a uma lista de permissões de ativos para o protocolo.
Update: Não consertado. A equipe de 1 polegada afirma:
OrderMixin
na verdade, não tem informações sobre tokens reais, poismakerAsset
etakerAsset
às vezes são proxies ou outros contratos intermediários e as informações sobre os tokens reais são armazenadas em alguns bytes arbitrários. Portanto, não há nenhuma maneira viável de restringir quais ativospermit
é chamado.
[N13] solhint
nunca é reativado
Em toda a base de código, há alguns solhint-disable
declarações, especificamente aquelas on-line 23 e on-line 41 of RevertReasonParser.sol
, que não terminam com solhint-enable
.
A favor da explicitação e de ser o mais restritivo possível ao desativar solhint
, considere usar solhint-disable-line
or solhint-disable-next-line
em vez disso, semelhante à linha 16 do mesmo arquivo.
Update: Fixo em pull request # 72.
[N14] Erros de digitação
A base de código contém os seguintes erros de digitação:
Além disso o projeto README
(fora do escopo desta auditoria) contém os seguintes erros de digitação:
Considere corrigir esses erros de digitação para melhorar a legibilidade do código.
Update: Fixo em pull request # 71 e pull request # 77.
[N15] Uso de uint
em vez de uint256
Para favorecer a explicitação, todas as instâncias de uint
deveria ser declarado como uint256
. Em particular, aqueles no for
loops em linhas 98 e 119 of OrderMixin.sol
e linhas 16 e 30 of PredicateHelper.sol
.
Update: Fixo em pull request # 70.
Conclusões
Foram encontrados 3 problemas de alta gravidade. Algumas mudanças foram propostas para seguir as melhores práticas e reduzir a superfície potencial de ataque.
- &
- 7
- Sobre
- Acesso
- Segundo
- Conta
- em
- Aja
- ações
- Adicional
- endereço
- Vantagem
- Todos os Produtos
- Permitindo
- já
- Apesar
- quantidades
- análise
- api
- abordagem
- argumentos
- por aí
- ativo
- Ativos
- auditor
- Back-end
- Começo
- ser
- MELHOR
- melhores práticas
- Pouco
- bots
- construir
- chamada
- Cuidado
- casos
- Causar
- Elo de corrente
- alterar
- a verificação
- Cheques
- código
- integrações
- condição
- confusão
- formação
- contém
- contract
- contratos
- Correções
- custos
- poderia
- Casal
- criador
- Moeda
- Atual
- dados,
- acordo
- Denial of Service
- Implantação
- Design
- desenvolvedores
- Desenvolvimento
- DID
- diferir
- diferente
- domínio
- duplo
- dinâmico
- Cedo
- borda
- encorajar
- especialmente
- ETH
- Evento
- eventos
- tudo
- exemplo
- exchange
- esperado
- vasta experiência
- Explorar
- Funcionalidades
- Campos
- Finalmente
- Primeiro nome
- Fixar
- Flexibilidade
- fluxo
- seguir
- encontrado
- cheio
- função
- fundos
- futuro
- jogo
- GAS
- Geral
- Dando
- ótimo
- guia
- Manipulação
- hash
- Hashing
- ter
- ajudar
- Alta
- altamente
- Como funciona o dobrador de carta de canal
- HTTPS
- HÍBRIDO
- identificar
- Impacto
- executar
- importante
- importador
- incluído
- Incluindo
- Crescimento
- aumentou
- info
- INFORMAÇÕES
- Infraestrutura
- insights
- intenção
- interesse
- Interface
- envolvido
- questões
- IT
- grande
- Maior
- conduzir
- principal
- Biblioteca
- Limitado
- Line
- Liquidez
- Listado
- listas
- local
- olhou
- pesquisa
- principal
- fabricante
- Fazendo
- mercado
- Mempool
- espelho
- modelo
- a maioria
- Mais populares
- nomeadamente
- Novos Recursos
- não fungível
- fichas não fungíveis
- oficial
- aberto
- Operações
- Opção
- oráculo
- ordem
- ordens
- Outros
- proprietário
- padrão
- Popular
- presente
- impedindo
- preço
- preços
- privado
- processo
- projeto
- projetos
- proteção
- protocolo
- fornecer
- fornece
- procuração
- público
- publicar
- compra
- qualidade
- aumentar
- Realidade
- reduzir
- confiança
- Denunciar
- Relatórios
- repositório
- Requisitos
- DESCANSO
- Resultados
- Retorna
- rever
- Risco
- rodadas
- Execute
- Sdk
- segurança
- Serviços
- conjunto
- compartilhado
- ações
- mudança
- semelhante
- simples
- pequeno
- smart
- Smart Contracts
- So
- solidez
- Spam
- especificamente
- Passar
- Spot
- começo
- Estado
- Declaração
- Unidos
- Status
- armazenamento
- estilo
- apresentado
- sucesso
- bem sucedido
- entraram com sucesso
- ajuda
- Suportado
- superfície
- Interruptor
- .
- Target
- teste
- ensaio
- testes
- Através da
- todo
- tempo
- juntos
- token
- Tokens
- pista
- Rastreamento
- transação
- Confiança
- único
- Atualizações
- us
- usabilidade
- USD
- USDT
- usuários
- utilidade
- valor
- Ver
- visibilidade
- esperar
- O Quê
- whitelist
- dentro
- sem
- Atividades:
- Equivalente há
- zero