ALTR security record

Calling a user provided address is dangerous, especially in a public function with no access control restriction. An attacker could deploy a malicious contract and use the vulnerable function to trigger a call to the malicious contract, potentially stealing user funds or causing other serious damages.

The function `grantRole` can be called by anyone as it has no access restriction. This enables anyone to call this and grant existing roles to any addresses. For the AltrFractions contract, there are `DEFAULT_ADMIN_ROLE` and `BURN_MANAGER_ROLE`.

The function `getFractionsAmountByPrice()` in the contract `AltrFractionsSale` loops through the `tiers` mapping, uses the input `price` to retrieve the amount of fractions that an NFT can be divided into. Since `getFractionsAmountByPrice` is a `view` function, the result to be returned is purely based on the on-chain data. That is to say, if the input arrays are not well checked in `constructor()` and `setTiers()`, `getFractionsAmountByPrice()` would return an incorrect result.

In contract AltrFeeManager, there is a function `setSaleInfo()` that can set the redemption fee token address and the first price. An external caller is required to be a privileged role, `TRADE_CHECKER_ROLE`, because of the `onlyRole()` modifier. However, the `TRADE_CHECKER_ROLE` is never assigned in the contract, which means no addresses can successfully call the function.

In the `setupSale()` function of the contract `AltrFractionsSale`, the local variable is a new-ed `TimedTokenSplitter`. The input parameter of the new `TimedTokenSplitter` has `fractionsAmount` as `0`. In the same contract `fractionsAmount` is also used as a required input fields for a new FractionsSale struct, and the `FractionsSale` is saved to the state variable `fractionsSales[currentSaleId]`. We feel like the `fractionsAmount` concept in the TimedTokenSplitter contract is different from the concept in the AltrFractionsSale contract. Our understanding is that `fractionsAmount` represents for the cap of a fraction sale of a NFT in AltrFractionsSale, while in TimedTokenSplitter, `fractionsAmount` is more like `fractionsSold`. We would like to confirm if our understanding is correct.

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. ```solidity=65 uint256 tokenPrice = redemptionToken.balanceOf(address(this)) / fractionsAmount; ``` ```solidity=69 fractionsPrice = tokenPrice * amounts[i]; ```

In the contract `AltrFeeManager`, the function `receive() external payable` can receive ether but the internal function `_transferEth` that can withdraw the fund is never called. In the contract `AltrTradeChecker`, the function `buyERC721(...) external payable` can receive ether but does not have a function to withdraw the fund. In addition, the `msg.value` of `buyERC721()` is neither recorded nor emitted via an event.

In the contract `AltrFractionsSale`, there is a function `setupSale()`, that can set up a new fraction sale. The requirement and initialization of two variables, `openingTime` and `closingTime`, caught our attention. The current require statements check that `openingTime > block.timestamp` and `closingTime > openingTime`. Our understanding is that there should be one or two constant variables like `MIN_OPENING_GAP` and `MIN_CLOSING_GAP` to make sure that a sale is not only open for a very short period of time, like one active block.

In contract `AltrFractionsSale`, the comment of the function `withdrawFractionsKept()` says that "The initiator can withdraw the fractions kept after the sale is closed and successful". However, the function implementation has only two checks: 1. the function caller is required to be the initiator 2. the fractions sold amount is required to be greater or equal to `saleMinFractions`, which is equivalent to checking that the sale is successful Neither of the two checks covers the case that the sale is required to be closed.

In the contract `AltrAllowList`, the `DEFAULT_ADMIN_ROLE` is granted to the contract deployer in the constructor function. However, the privileged role is never used.