QA Report

[code423n4]

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

Impact - LOW

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

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/fei-protocol/xTRIBE/blob/989e47d176facbb0c38bc1e1ca58672f179159e1/src/xTRIBE.sol#L134

https://github.com/fei-protocol/xTRIBE/blob/989e47d176facbb0c38bc1e1ca58672f179159e1/src/xTRIBE.sol#L137

Tools Used

Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

C4-002 : 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/fei-protocol/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L80

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.

C4-003 : PREVENT DIV BY 0

Impact

On several locations in the code precautions are taken not to divide by 0, because this will revert the code. However on some locations this isn’t done.

Especially in the claim function div(initialStake - claimedInitialStake) which isn’t checked.

That will cause to revert on the claim function.

Proof of Concept

1. Navigate to the following contract,

"https://github.com/fei-protocol/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L38"

2. rewardsCycleLength can be same and the substraction operation will be equal to zero therefore div by zero will occur.

Tools Used

Review

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.

C4-004 : # Missing zero-address check in the setter functions and initiliazers

Impact

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/fei-protocol/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L38

https://github.com/fei-protocol/xTRIBE/blob/989e47d176facbb0c38bc1e1ca58672f179159e1/src/xTRIBE.sol#L33

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors:
require(newAddr != address(0));.

C4-005 : Owner does not have set function

Impact

During the code review, It has been observed that owner is only set in the constructor. It is not inherited from the Ownable (Openzeppelin contract.) If the owner is deployed mistakenly with zero address/wrong address, access will be lost.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/fei-protocol/xTRIBE/blob/989e47d176facbb0c38bc1e1ca58672f179159e1/src/xTRIBE.sol#L35

Tools Used

Code Review

Recommended Mitigation Steps

Consider inheriting contract from Openzeppelin Ownable contract.