aelinxyz / aelin Goto Github PK

Hey Aelin team,

I hope you are fine and doing good, I have been reading your contracts since I started looking at your audits done by different individuals and companies, many things are being pointed out by the auditors. However, one Medium BUG I would like to address here at https://github.com/AelinXYZ/aelin/blob/53710152d3746cbfc5337e88a3f01694d5b26999/contracts/Owned.sol.

BUG:

The contract work as expected as at the start it checks for address(0), owner can never be address(0) fair enough. However, this restriction is lacking when changing the owner functionality comes in at nominateNewOwner.

In order to test it, try to deploy the contract with address(0) which would revert due to require(_owner != address(0), "Owner address cannot be 0");, now deploy the contract normally and pass address(0) at nominateNewOwner function. This makes the possibility of making invalid address the owner of the contract.

Possible Recommendation:

add check at address(0) as:

function nominateNewOwner(address _owner) external onlyOwner {
        require(_owner != address(0), "No zero address");
        nominatedOwner = _owner;
        emit OwnerNominated(_owner);
    }

Thank you,

Regards,
newfolder

NOTE that we might not want to start on this ticket until they are supported across every EVM network we support. Delegate cash looks like they have better documentation and support more EVM networks at the moment. We can wait until they support every network we are on

Right now the contract workflow works for every edge case but it is not optimized for some common use cases. This is really bad for mainnet deals when gas is expensive. we have seen some users unhappy with the transaction costs even when they are necessary for a given pool

1. if there is no deallocation allowed and no purchase raise minimum but there is a vesting schedule we should be able to skip the settle transaction
2. if there is no deallocation allowed and no purchase raise minimum and there is also no vesting period, we can just exchange the tokens directly when they send the funds in. no need for settling or claiming.
3. if deallocation is allowed but there is no vesting period, we can exchange the tokens when settle is called

Implementation of AELIP-30

This AELIP proposes adding the ability for deal creators to configure multiple vesting schedules within the same deal to be sold at different prices. The purchaser will be able to choose the vesting schedule they prefer for the corresponding token price.

An old PR was started here but never finished.

Need to implement https://aelips.aelin.xyz/aelips/aelip-34/.

This AELIP proposes to create a new allocation mechanism, commonly referred to as a "Laminar allocation", for investors in NFT gated deals when there is excess interest in a capped deal. In the proposed allocation, smaller investors are deallocated less than larger investors to ensure a wider distribution of deal tokens.

Please note that for this feature to be implemented and the "cut off value" to be calculated, all the deposits need to be sorted in ascending order. This is not very gas friendly knowing that a pool could get 1000+ deposits. A first guess would be to maintain and update a list every time a new deposit is made. Any element (deposit/wallet) of this list needs to be easily accessible to make sure we don't run out of gas. An implementation of a LinkedList in Solidity could definitely work.

Right now, we support ERC721 contracts but we do not support contracts that mint multiple collections on a single contract address like POAP. We need to modify NFT gated deals to add support for POAP so that you can whitelist different ranges of token ids on a contract instead of supporting the entire contract. this can be used to support sub collections of any ERC721 that use consecutive IDs

We need to add proper commenting formatting to all code in the Aelin repo

We can just do this for all the contracts related to upfront deals for now. the Aelin Pools is less popular and we will slowly stop supporting these contracts unless we see an increase in demand for this product in the coming months

NOTE we might want some of the standalone contracts in the repo updated as well depending if we will reuse them. TODO: look into which contracts outside of the AelinUpFrontDeal.sol and related libraries and contracts need to be updated

if (!nftCollectionRules.purchaseAmountPerToken && nftCollectionRules.purchaseAmount > 0) {
require(!_data.nftWalletUsedForPurchase[_collectionAddress][msg.sender], "wallet already used for nft set");

this logic in AelinNFTGating.sol is something that we have never used and the logic does not make sense. We should remove it from the logic set. There is the option to have it so that a wallet can only participate once and all their NFTs become blacklisted. the reason it doesn't make sense is we need to pass in the token ids to be blacklisted. if a pool is used with this setting it just incentivizes people to send their NFTs to different wallets which is not ideal. we should just remove it

Hello developers. We are utilizing some automated tools to detect potential dependency issues. If there are any inaccuracy, we would greatly appreciate your corrections and feedback.

We noticed that there is a known problem/bug in the contract libraries/NftCheck.sol. The NftCheck contract use solidity 0.8.6 and abi.decode. According to GHSA-qh9x-gcfh-pcrw, this contract may revert instead of returning false. The influenced functions are functions invoking _supportsInterface. It could be upgraded to the fixed version or the latest like the OZ library.

This issue may not directly cause security risks, but it can influence users by malicious data or someone who forked this repository. We known that possible fixes may have to be in the next major version. However, we hope that the security advisory could be in contract comments or documentation to facilitate users' understanding of potential issues and monitoring of actual behaviors.