Foundation contest - defsec's results

C4-001 : Use of ecrecover is susceptible to signature malleability

Impact - LOW

The ecrecover function is used in permit() to recover the address from the signature. The built-in EVM precompile ecrecover is susceptible to signature malleability which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketPrivateSale.sol#L171

Tools Used

None

Recommended Mitigation Steps

Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function.

C4-002 : Missing Conditional Check In the Allowance

Impact - LOW

During the code review, It has been observed that If the allowance is given maximum uint. The check should be nice to have check if the current allowance is maximum.

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/FETH.sol#L212"
2. The max allowance check has not been checked on the function.

Ensure that is the required checks are compatible with Openzeppelin ERC20.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L336

Tools Used

None

Recommended Mitigation Steps

Implement the following check in the related function.

if (allowed[from][msg.sender != type(uint256).max)

Reference

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L336

C4-003 : Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Impact - LOW

The buyFromPrivateSaleFor function of nftContract is called when transferring the nft to the person. However, this function does not check whether the recipient is aware of the ERC721 protocol and calls _transfer directly. If the recipient is a contract not aware of incoming NFTs, then the transferred NFT would be locked in the recipient forever.

Proof of Concept

1. Navigate to the following contract.

2. transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketPrivateSale.sol#L177

Tools Used

Code Review

Recommended Mitigation Steps

Use the _safeTransfer function instead, which checks if the recipient contract implements the onERC721Received interface to avoid loss of NFTs.

C4-004 : ReentrancyGuardUpgradeable contract is not initialized

Impact - LOW

The ReentrancyGuardUpgradeable contract is not initialized through constructor.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/FNDNFTMarket.sol#L73

Tools Used

Code Review

Recommended Mitigation Steps

Consider initializing the Upgradeable contract in the constructor/initializer function.

C4-005 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/FNDNFTMarket.sol#L105

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/FoundationTreasury.sol#L61

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketAuction.sol#L22

2. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-006 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L389

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.