6 de dezembro de 2021
Introdução
A Tally A equipa pediu-nos para rever e auditar um conjunto de contratos com o objetivo final de melhorar os contratos de governação comum e dar-lhes mais flexibilidade. Analisamos o código e agora publicamos nossos resultados.
Visão geral do sistema
O sistema depende de três contratos principais:
- A
SafeGuard
modelo de contrato. Este contrato é oadmin
de umaTimelock
contrato e adiciona uma camada de funções modulares ao longo doTimelock
ações. Este contrato define diversas funções que têm responsabilidade e acesso separados sobre o estado de uma proposta (enfileiramento, cancelamento e execução). - A
SafeGuardFactory
contrato implanta um novoSafeGuard
e um novo correspondenteTimelock
contrato. Em seguida, ele define o endereço do timelock dentro doSafeGuard
contrato e registra o novoSafeGuard
noRegistry
. - A
Registry
que contém uma lista de implantadosSafeGuards
com seus correspondentes número da versão.
A SafeGuardFactory
basicamente gerará um novo SafeGuard
sempre que for chamado. A SafeGuard
é um envoltório Timelock
operações que adiciona funções diferentes e separadas para cada ação. As funções são estruturadas através do uso do OpenZeppelin AccessControlEnumerable
contrato dando mais flexibilidade e compatibilidade às carteiras multisig e casos de uso de negócios onde vários endereços podem ter a mesma função compartilhada.
Update: No PR # 10, a equipe Tally decidiu remover o Registry
contrato. A lista de salvaguardas implantadas agora está armazenada no SafeGuardFactory
contrato, no safeGuards
conjunto enumerável, juntamente com sua versão armazenada no safeGuardVersion
mapeamento.
Setores
A SafeGuard
contrato define as seguintes funções:
Update: A CREATOR_ROLE
role foi removida como uma correção para o problema L02.
Objetivo
Auditamos o commit b2c63a9dfc4090be13320d999e7c6c1d842625d3
da safeguard
repositório. No escopo estão os contratos inteligentes no contracts
diretório. No entanto, o mocks
diretório foi considerado fora do escopo.
Pressupostos
O sistema não foi feito para ser atualizável. O Registry
endereço está definido no construtor da SafeGuardFactory
e cada SafeGuard
pode ter o Timelock
definir apenas uma vez. Isto significa que se um novo Registry
é implantado um novo SafeGuardFactory
também deve ser implantado. Se o Timelock
mudanças de implementação, o antigo SafeGuard
s se tornarão obsoletos e novos terão que ser implantados.
Além disso, o sistema depende fortemente da implementação de um Timelock
contract que foi considerado fora do escopo desta auditoria. A equipe ainda não finalizou a implementação do Timelock
contrato. No momento da redação deste relatório, a implementação do timelock do Composto está sendo usado no projeto.
A base de código foi auditada por dois auditores durante uma semana e aqui apresentamos nossas descobertas.
Gravidade crítica
Nenhum.
Alta gravidade
[H01] ETH pode ser bloqueado dentro do Timelock
contract
A Tally
equipe originalmente baseou suas implementações no terreno do GovernorBravoDelegate
Contrato composto.
Durante esta auditoria, o Tally
equipe descobriu uma limitação no governador do Compound onde a ETH enviou diretamente para o Timelock
não está disponível para utilização em propostas de governação e, embora não esteja permanentemente bloqueado, requer uma solução alternativa elaborada para ser recuperado.
Isso ocorre porque a implementação do governador exige que todo o valor de uma proposta seja anexado como msg.value
pela conta que aciona a execução, não utilizando de forma alguma o Timelock
Fundos ETH.
A mesmo problema foi posteriormente identificado no SafeGuard
implementação e a equipe está ciente do problema e está em processo de corrigi-lo.
Ao corrigir o problema, considere usar a abordagem adotado pela biblioteca OpenZeppelin para o mesmo problema.
Update: Corrigido no commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory pode ser congelado
A Registry
contrato tem como objetivo acompanhar todos os SafeGuards
que o SafeGuardFactory
produz. Tem o externo register
função que é usado para esse fim.
Ao mesmo tempo, o SafeGuardFactory
tem, em seu construtor, a atribuição do local registry
valor para o valor de entrada. Não há possibilidade de alterar o valor do registry
variável e por esta razão, se um novo Registry
for implantada, uma nova fábrica também deverá ser implantada.
A SafeGuardFactory
tem o createSafeGuard
função, responsável pela primeira implantação de um novo SafeGuard
, Em seguida um novo Timelock
com o endereço do SafeGuard
as admin
, então definindo o timelock
variável do SafeGuard
contrato e finalmente registrando o SafeGuard
no registro.
A questão é que qualquer chamada para createSafeGuard
pode ser forçado a falhar por um invasor que possa registrar diretamente o endereço determinístico do novo SafeGuard
antes de sua criação. Sempre que um contrato cria um new
instância, seu nonce é aumentado e o endereço de onde a nova instância do contrato seria implantada pode ser determinado pelo endereço original do contrato e seu nonce. Portanto, um invasor pode pré-calcular muitos dos endereços onde o novo SafeGuards
serão implantados e registrar esses endereços no Registry
chamando o register
função. Isso resultaria nas chamadas para createSafeGuard
para reverter uma vez que o Registry
já contém o endereço.
Para evitar que intervenientes externos chamem publicamente a Register
contrato, considere restringir o acesso ao register
função para aceitar chamadas exclusivamente do SafeGuardFactory
.
Update: Fixo em PR # 10. A equipe Tally removeu o Registry
contrato.
Gravidade média
Nenhum.
Baixa gravidade
[L01] Código comentado
A Registry
contrato inclui uma linha de código comentada. Para melhorar a legibilidade, considere removê-lo da base de código.
Update: Fixo em PR # 10 e comprometer 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] O administrador do SafeGuard pode atribuir a função de criador a qualquer endereço
A SafeGuard
contrato define o papel de um CREATOR_ROLE
que, como o nome sugere, é atribuído ao criador da salvaguarda.
Contudo, ao invocar que o grantRole
função do AccessControlEnumerable
contract na biblioteca de contratos OpenZeppelin, um administrador pode conceder essa função a qualquer endereço. Isto poderia causar confusão porque o criador do SafeGuard
só pode ser o SafeGuardFactory
.
Em toda a base de código, esta função foi usada apenas para restringir a interação dos usuários com o setTimelock
função da SafeGuard
contrato. Por design, o sistema garante que setTimelock
função pode ser chamado apenas uma vez, a partir de dentro do SafeGuardFactory
contract.
Considere remover o CREATOR_ROLE
papel desde o SafeGuard
contrato e usando o onlyOwner
mudança no setTimelock
função.
Update: Fixo em PR # 10.
[L03] Definição e implementação de interface incorretas
A ISafeGuard
interface não define o queueTransactionWithDescription
função implementado no SafeGuard
contrato e, ao mesmo tempo, define o __abdicate, __queueSetTimelockPendingAdmin e __executeSetTimelockPendingAdmin funções, mas elas não são implementadas.
Para melhorar a correção e consistência na base de código, considere refatorar o ISafeGuard
interface para corresponder exatamente ao SafeGuard
implementação.
Update: Corrigido no commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Documentos ausentes
Alguns dos contratos e funções na base de código não possuem documentação. Por exemplo, algumas funções no SafeGuard
contrato.
Além disso, alguns docstrings usam linguagem informal, como aquele acima do setTimelock
Funciona no SafeGuard
contract.
Isto dificulta a compreensão dos revisores sobre a intenção do código, o que é fundamental para avaliar corretamente não só a segurança, mas também a correção. Além disso, os docstrings melhoram a legibilidade e facilitam a manutenção. Devem explicar explicitamente o propósito ou intenção das funções, os cenários em que podem falhar, os papéis autorizados a chamá-las, os valores devolvidos e os eventos emitidos.
Considere documentar minuciosamente todas as funções (e seus parâmetros) que fazem parte da API pública dos contratos. As funções que implementam funcionalidades confidenciais, mesmo que não sejam públicas, também devem ser claramente documentadas. Ao escrever doutrinas, considere seguir o Formato de Especificação Natural Ethereum (NatSpec).
Update: Parcialmente fixado em PR # 10. Documentações adequadas foram adicionadas a várias funções em toda a base de código. No entanto, além das alterações atuais, considere fazer as seguintes alterações:
- Adicionar
description
como o@param
na doutrina acimaqueueTransactionWithDescription
função - Adicionar
@param
no doutrina acima docreateSafeGuard
funcionam emSafeGuardFactory
contract - Adicionar
@return
em docstrings acima das funções emSafeGuardFactory
contrato.
[L05] Código inútil ou repetido
Existem locais na base de código onde o código é repetido ou não é necessário. Alguns exemplos são:
- Linhas 29-32 da
Registry
contrato são inúteis, porque o_add
função doEnumerableSet
contract já realiza essas verificações contra os valores já definidos. - Linhas 62, 67, 73 e 78 da
SafeGuard
contrato estão todos repetindo exatamente a mesma operação. Considere encapsulá-lo em uma função interna para evitar a duplicação de código. - Linhas 62-63 e 67-68 of
SafeGuard
são repetidos. Considere encapsulá-los em uma única função interna. - O uso de
gasleft
para especificar quanto gás deve ser encaminhado na chamada da funçãoexecuteTransaction
é desnecessário. Isso porque, nesse ponto de execução, todo o gás que sobrar será utilizado para dar continuidade à execução. Se isso não for explícito, considere remover ogas
parâmetro da chamada.
Considere aplicar a correção sugerida para produzir um código mais limpo e melhorar a consistência e modularidade na base de código.
Update: Fixo em PR # 10 e comprometer 7fd27df16fc879d990d36a167a0b6e719e578558
.
Notas e informações adicionais
[N01] Estilo inconsistente
Existem alguns lugares na base de código onde diferenças de estilo afetam a legibilidade, tornando mais difícil a compreensão do código. Alguns exemplos são:
- A
Registry
contrato usa estilos diferentes para docstrings em todo o contrato. - A
SafeGuard
contrato está emitindo um evento quandoqueueTransactionWithDescription
é chamado, mas nenhum evento é emitido em outras funções que lidam com transações. - No
SafeGuard
contrato, às vezes valor é usado como parâmetro nomeado e às vezes _valor é usado.
Levando em consideração o valor que um estilo de codificação consistente agrega à legibilidade do projeto, considere impor um estilo de codificação padrão com a ajuda de ferramentas linter, como Solhint.
Update: Fixo em PR # 10 e comprometer 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Licença ausente
Os contratos a seguir na base de código não possuem um identificador de licença SPDX.
Para silenciar os avisos do compilador e aumentar a consistência em toda a base de código, considere adicionar um identificador de licença. Ao fazer isso, considere referir-se a spdx.dev diretrizes.
Update: Fixo em PR # 10 e comprometer 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] A dependência do contrato OpenZeppelin não está fixada
Para evitar comportamentos inesperados no caso de alterações importantes serem lançadas em atualizações futuras do Biblioteca de contratos OpenZeppelin, considere fixar a versão dessa dependência no package.json arquivo.
Update: Fixo em PR # 10.
[N04] A versão do compilador Solidity não está fixada
Em toda a base de código, considere fixar a versão do Compilador de solidez para sua última versão estável. Isso deve ajudar a evitar a introdução de bugs inesperados devido a versões futuras incompatíveis. Para escolher uma versão específica, os desenvolvedores devem considerar os recursos do compilador necessários ao projeto e a lista de bugs conhecidos associado a cada versão do compilador Solidity.
Update: Fixo em PR # 10.
[N05] Erro de digitação
Em vários casos ao longo da base de código, a palavra role
está escrito incorretamente como rol
. Um exemplo está em a doutrina dentro do constructor
da SafeGuard
contract.
Considere corrigir esses erros de digitação para melhorar a legibilidade do código.
Update: Parcialmente fixado em PR # 10. Enquanto a ortografia de role
foi corrigido, o comentário “definir a função de administrador como um endereço de administrador definido” deve ser “definir a função de administrador como um endereço de administrador definido”. Além disso, “executar” está escrito incorretamente no SafeGuard
contrato em linha 69, linha 82, linha 96 e linha 110 e “disponível” está escrito incorretamente em linha 70, linha 83, linha 97, linha 111. Além disso, considere substituir palavras informais como "vai" in SafeGuard
contrato com alternativas formais como “ir para”.
[N06] Declare uint como uint256
Existem diversas ocorrências na base de código onde variáveis são declaradas de uint
tipo de dados em vez de uint256
. Por exemplo, o eta
variável no QueueTransactionWithDescription
evento da SafeGuard
contrato.
Para favorecer a explicitação, todas as instâncias de uint
deveria ser declarado como uint256
.
Update: Fixo em PR # 10 e comprometer 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Importação não utilizada
A SafeGuard
contrato importa o console
contrato, mas nunca o usa.
Para melhorar a legibilidade do código, considere remover quaisquer importações não utilizadas.
Update: Fixo em PR # 10.
Conclusões
Uma vulnerabilidade alta e várias outras vulnerabilidades menores foram encontradas e recomendações e correções foram sugeridas.
- &
- Acesso
- Conta
- em
- Açao Social
- ações
- Adicional
- endereço
- admin
- Todos os Produtos
- já
- Apesar
- api
- abordagem
- por aí
- auditor
- Basicamente
- ser
- erros
- negócio
- chamada
- casos
- Causar
- alterar
- carregar
- código
- Codificação
- comum
- Compound
- confusão
- consideração
- contém
- continuar
- contract
- contratos
- poderia
- criador
- Atual
- dados,
- lidar
- Design
- desenvolvedores
- diferente
- descoberto
- Elaborar
- ETH
- Evento
- eventos
- exemplo
- fábrica
- Funcionalidades
- Finalmente
- Primeiro nome
- Fixar
- Flexibilidade
- encontrado
- função
- fundos
- futuro
- GAS
- Dando
- meta
- governo
- Governador
- orientações
- ter
- ajudar
- SUA PARTICIPAÇÃO FAZ A DIFERENÇA
- Alta
- Como funciona o dobrador de carta de canal
- HTTPS
- implementado
- Crescimento
- aumentou
- Interface
- IT
- conhecido
- língua
- mais recente
- Biblioteca
- Licença
- Line
- Lista
- local
- trancado
- olhou
- Fazendo
- Match
- modulares
- Multisig
- Outros
- presente
- processo
- projeto
- proposta
- público
- publicar
- cadastre-se
- Releases
- Denunciar
- repositório
- Resultados
- rever
- segurança
- conjunto
- contexto
- compartilhado
- smart
- Smart Contracts
- solidez
- Estado
- estilo
- .
- Através da
- todo
- tempo
- ferramentas
- pista
- Transações
- Atualizações
- us
- usuários
- valor
- vulnerabilidades
- Carteiras
- semana
- QUEM
- dentro
- palavras
- escrita