Introducción
Se le solicitó a CoinFabrik que audite los contratos para el token ERC721 de CryptoCup. En primer lugar, proporcionaremos un resumen de nuestros descubrimientos y, en segundo lugar, mostraremos los detalles de nuestros hallazgos.
Resumen
Los contratos auditados son del repositorio CryptoCup . La auditoría se basa en la confirmación 7b55e9afe2f9818d523e62a89141702c175a8504 y se actualizó para reflejar los cambios en dd0097646b490e82265f5bf5e96ba82875ef3898 .
Los contratos auditados son:
- AccessControlLayer.sol: Modificadores de llamada de función privilegiada.
- DataLayer.sol: almacenamiento de contrato.
- CryptocupDataSource.sol: implementa callbacks Oraclize.
- CryptocupToken.sol: implementación del token ERC721.
- GameLogicLayer.sol: lógica de cálculo de puntos.
- CoreLayer.sol: Funciones adicionales relacionadas con Tokens.
- DataSourceInterface.sol: Interfaz para CryptocupDataSource.sol.
- DataSourceCallbackInterface.sol: interfaz de devolución de llamada GameLogic de oraclize.
- SafeMath.sol: Desbordamiento de operaciones enteras comprobadas.
- ERC721.sol: interfaz ERC721.
Se realizaron los siguientes análisis:
- Mal uso de los diferentes métodos de llamada: call.value (), send () y transfer ().
- Errores de redondeo de enteros, desbordamiento, subdesbordamiento y uso relacionado de las funciones de SafeMath.
- Viejos pragmas de la versión del compilador.
- Condiciones de carrera tales como ataques de reentrada o carreras frontales.
- Mal uso de las marcas de tiempo del bloque, asumiendo que cualquier cosa que no sea estrictamente creciente.
- Contrato de ataques de softlocking (DoS).
- Costo de gas potencial de las funciones que están por encima del límite de gas del bloque.
- Faltan calificadores de funciones y su uso incorrecto.
- Fallback funciona con un costo de gas más alto que el máximo permitido por transferencia o envío.
- Código fraudulento o erróneo.
- Complejidad de la interacción del código y el contrato.
- Error o falta de manejo de errores.
- Uso excesivo de transferencias en una sola transacción en lugar de utilizar patrones de retiro.
- Insuficiente análisis de los requisitos de entrada de la función.
Descubrimientos Detallados
Severidad Crítica
La eliminación del token usa un índice incorrecto en la función de transferencia
En esta función:
function _transfer(address fromAddress, address toAddress, uint256 tokenId) internal
{
tokensOfOwnerMap[toAddress].push(tokenId);
ownerOfTokenMap[tokenId] = toAddress;
delete tokensOfOwnerMap[fromAddress][tokenId];
delete tokensApprovedMap[tokenId];
}
La variable tokensOfOwnerMap [fromAddress] es una matriz que contiene los tokens de fromAddress pero esa matriz no es indexable por tokenId, simplemente los almacena. Esto debe solucionarse buscando dicho tokenId en la matriz para obtener el índice adecuado para la eliminación.
Esto se corrigió en commit d299c8bddce19014d6b490cc4d8447305fc3f01a
Severidad Media
La precondición de la retirada de emergencia es contraria a la especificación
Este modificador:
modifier hasFinished()
{
require(gameFinishedTime + (90 days) >= now);
_;
}
Usado por esta función:
//let the admin cashout entire contract balance after 90 days after game has finished.
function finishedGameWithdraw() external onlyAdmin hasFinished
{
uint256 balance = address(this).balance;
adminAddress.transfer(balance);
}
Comprueba lo contrario de lo que se afirma en el comentario, la condición es verdadera desde el momento en que gameFinishedTime se asigna hasta 90 días después de que el juego ha terminado.
La condición debe ser negada, al tiempo que se tiene en cuenta que gameFinishedTime comienza en 0, lo que también hace que la condición sea verdadera antes de que se le asigne.
Esto se corrigió en commit d299c8bddce19014d6b490cc4d8447305fc3f01a
Algunas funciones pueden tener problemas de costo de gas
Actualmente, algunas funciones como esta::
function resetWinners() external onlyAdmin checkState(pointsValidationState.LimitCalculated)
{
sortedWinners.length = 0;
}
O esta:
function withdrawPrize() external checkState(pointsValidationState.Finished)
{
uint256 prize = 0;
uint256[] memory tokenList = tokensOfOwnerMap[msg.sender];
for (uint256 i = 0; i < tokenList.length; i++)
{
prize += tokenToPayoutMap[tokenList[i]];
tokenToPayoutMap[tokenList[i]] = 0;
}
require(prize > 0);
msg.sender.transfer((prizePool.mul(prize)).div(1000000));
}
Puede tener un alto costo de gas porque es lineal con respecto a la longitud de la matriz. Si los costos de gas son mucho más altos que el límite de bloque promedio, no podrá llamar a estas funciones. Considere poner un límite a estas matrices o limitar el número de elementos que procesa en una sola llamada.
Esto se corrigió en el commit d299c8bddce19014d6b490cc4d8447305fc3f01a y confirma b959f4e68dc16d107551638185fbbb3b924c2f1d
Severidad Mínima
Algunas funciones necesitan documentación
Algunas funciones como la mayoría en GameLogicLayer necesitan la documentación adecuada. Estas son funciones complejas de las cuales no es fácil inferir la intención.
Esto fue abordado por commits múltiples
Advertencias de Solidity
Hay algunas advertencias de solidity irrelevantes que deben corregirse ya que pueden ocultar advertencias más importantes.
Uso de matemática segura no exhaustiva
Algunas integer aritméticos aún no tienen verificaciones de overflow, esto puede ser peligroso ya que los overflows y underflows son relativamente comunes. Considere reemplazar todos los casos con sus operaciones SafeMath equivalentes.
Mejoras
La función getPayoutDistributionId no es necesaria
getPayoutDistributionId se usa para obtener el id de índice correspondiente para la distribución de pagos:
function getPayoutDistributionId () internal view returns(uint256 id)
{
if(tokens.length < 101)
{
id = 0;
}
else if(tokens.length < 201)
{
id = 1;
}
else if(tokens.length < 301)
{
id = 2;
}
else if(tokens.length < 501)
{
id = 3;
}
else if(tokens.length < 1001)
{
id = 4;
}
else if(tokens.length < 2001)
{
id = 5;
}
else if(tokens.length < 3001)
{
id = 6;
}
else if(tokens.length < 5001)
{
id = 7;
}
else if(tokens.length < 10001)
{
id = 8;
}
else if(tokens.length < 25001)
{
id = 9;
}
else
{
id = 10;
}
}
But this is not necessary since setPayoutDistributionId already assigns the corresponding payoutDistribution conditionally. The other two arrays lastPosition and superiorQuota using said id can get the same treatment in the setter function.
Pero esto no es necesario ya que setPayoutDistributionId ya asigna la distribución de pago correspondiente condicionalmente. Los otros dos arrays lastPosition y superiorQuota utilizando dicho id, pueden obtener el mismo tratamiento en la función setter.
Esto se mejoró en commit d299c8bddce19014d6b490cc4d8447305fc3f01a
Conclusión
La idea principal de un token ERC721 es simple y los contratos son simples. Si bien hubo algunos errores críticos, todos tuvieron soluciones simples que fueron abordadas por el equipo de CryptoCup.
Este artículo también esta disponible en inglés
Permalink