Macondo security record

A state variable is shadowing another component defined in a parent contract. Variable `vrfCoordinator` in `RandomOracleConsumer` shadows the variable `vrfCoordinator` in `VRFConsumerBaseV2`. ```solidity=22 address vrfCoordinator; ``` ```solidity=99 address private immutable vrfCoordinator; ```

In the contract `NFTStore`, the function `_sale()` requires that the `price` parameter is strictly greater than 1. However, the function `_setSaleConfigPrice()` allows setting `defaultConfig.price = 1`. \ This could cause unexpected errors.

The function `_setSaleConfigSaleTime()` prevents having `_saleStartTime > _saleEndTime` however the two parameters can still be equal.

When function `_sale()` is called, if the caller sent more `BNB` than `price`, then the surplus will be sent back to the caller. This causes an external call to an untrusted contract during the execution of a sensitive function. \ Since the `price` is supposed to be known before calling the function, not requiring that the exact amount is paid seems to introduce unnecessary risks into the protocol.

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. *This finding is considered minor because the reentrancy only causes out-of-order events.*

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=54 vrfCoordinator = m_vrfCoordinator; ``` - `m_vrfCoordinator` is not zero-checked before being used. --- ```solidity=51 withdrawAddress = _withdrawAddress; ``` - `_withdrawAddress` is not zero-checked before being used. --- ```solidity=137 withdrawAddress = _withdrawAddress; ``` - `_withdrawAddress` is not zero-checked before being used.

In the contract `NFTStore`, the function `_sale()` does not check that `to != address(0)`. \ This could potentially allows to bypass the check: ```solidity=90 if (soldList[tokenId] != address(0)) { revert(string(abi.encodePacked("token already sold"))); } ``` in case of a user tries to resell a token.

The function `getRandomWords()` is a `view` function that only the `owner` of the contract can call.

The function `_setSaleConfigPrice()` checks that `_salePrice <= 0`. `_salePrice` being an `uint256` it cannot be less than zero.

State variables that never change should be declared as `constant` to save gas. ```solidity=35 uint32 callbackGasLimit = 300000; ``` - `callbackGasLimit` should be declared `constant`. --- ```solidity=38 uint16 requestConfirmations = 3; ``` - `requestConfirmations` should be declared `constant`. --- ```solidity=42 uint32 numWords = 10; ``` - `numWords` should be declared `constant`.