Skip to content

Latest commit

 

History

History
86 lines (51 loc) · 3.6 KB

avoiding_common_attacks.md

File metadata and controls

86 lines (51 loc) · 3.6 KB

Avoiding Common Attacks

Smart contracts were audited for security vulnerabilities using static code analysis tool such as MythX. Reports were taken into account to fix data and methods visibility, and other recommendations. Following the checklist details some consideration taken into account during security analysis of Nifties exchange smart contracts.

Reentrancy

All contract functions were implemented taking into account the pattern of finishing all internal work (ie. state changes) first, and then call any required external function.

Transaction Ordering or Front-Running

We handle a replayNonce record within the CollectibleMetaTxRelayer contract so it cannot suffer of transactions re-ordering or reply attacks.

Integer Over / Underflow

All uint calculations are being processed using the SafeMath library from OpenZeppelin.

Denial of Service

The StampCollectible contract implements the pull vs push design pattern to transfer ETH for selling stamps. Therefore NFTs cannot be locked forever by bad actors, such as an smart contract wallet with a fallback function that always revert().

Poison Data

Require statements are always used before processing internal work.

Exposed Functions

Some functions exposition were fixed after using static analysis tools.

Malicious Admins

Only contract owners are able to add/remove admins in certain smart contracts such as the StampCollectible. which inherits from the MinterRole.

Failed Sends

The relayer guarantees to include enough amount of ETH and gas to airdrop stamps.

Tx.Origin Problem

All smart contracts always use msg.sender

Incorrect use of Cryptography

CollectibleMetaTxRelayer contract uses OpenZeppelin ECDSA Library to call the .toEthSignedMessageHash().recover() and obtain the address of the signer.

Avoid state changes after external calls

Currently, all smart contacts perform external call to view functions.

Don't delegatecall to untrusted code

Besides audited libraries used within the code, no smart contract needed to use the delegatecall functionality.

Pending warning on MythX

A floating pragma is set SWC-103 was ommited from the warnings due to the dApp is not going to production at the moment.

Vulnerabilities within the stamp market price estimator

Following you can see the function implementation to calculate the NFT price based on demand and randomness based on past block hashes:

function getNFTPrice(uint256 _index) internal view returns (uint16) {
    Stamp memory stamp = stamps[_index];
    uint256 x = 0;
    for(uint8 depth = priceDepth; depth > 0 ; depth--) {
      bytes32 blockHash = blockhash(block.number-depth);
      x += convert(blockHash[_index * 2]) << 8 | convert(blockHash[_index * 2 + 1]);
    }
    return uint16(x / priceDepth) + uint16(stamp.inWild);
}

During static analysis with MythX, the following warnings were shown:

125:30  warning  Potential use of a weak source of randomness "blockhash"     SWC-120
125:40  warning  Potential use of a weak source of randomness "block.number"  SWC-120

Using past or the current block hashes through "block.number" as a source of
    randomness is predictable. The issue can be ignored if this is unrelated to
    randomness or if the usage is related to a secure implementation of a
    commit/reveal scheme.

Even if the artificial market generated by the on-chain exchange functionality can be gamed by a bad actor in a certain degree, the functionality of the dApp game for onboarding purposes is fine as long as it does not handle large amounts of ETH on the Prize pot.