Open code423n4 opened 2 years ago
C4-001 : Incompatibility With Rebasing/Deflationary/Inflationary tokens
Impact - LOW
Maple protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
Proof of Concept
- Navigate to the following contract.
https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L162 https://github.com/maple-labs/mpl-migration/blob/main/contracts/Migrator.sol#L26
We don't plan on supporting these types of tokens, not addressing.
C4-002 : The Contract Should Approve(0) first
Impact - LOW
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
IERC20(token).approve(address(operator), 0); IERC20(token).approve(address(operator), amount);
This is not necessary since MPL does not use the USDT approve functionality, not a valid issue.
C4-003 : 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
- Navigate to the following contract.
https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L90
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.
Not a valid issue, block.timestamp is required in this case and miner manipulation wouldn't have any adverse consequences.
C4-004 : Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
Impact - LOW
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
- Navigate to the following contract.
- transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.
https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L162 https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L181
ERC20Helper is Maple's implementation of SafeTranfer/SafeTransferFrom: https://github.com/maple-labs/erc20-helper so this is not a valid issue.
C4-005 : Consider making contracts Pausable
Impact - LOW
There are many external risks so my suggestion is that you should consider making the contracts pausable, so in case of an unexpected event, the admin can pause transfers.
https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L99
We don't want to make transfers pauseable in the RDT contract, it gives too much power to the admin.
C4-006 : Critical changes should use two-step procedure
Impact - NON CRITICAL
The critical change should be completed with the two step. For instance, migrate function can be implemented with the two step procedure. (accept migrating)
Proof of Concept
- Navigate to the following contracts.
https://github.com/maple-labs/mpl-migration/blob/main/contracts/Migrator.sol#L20
We don't want to add this, doesn't make sense to do in migration. Not a valid issue.
C4-007 : # USE SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE
Impact - LOW
Note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().
Proof of Concept
Not a valid issue since MPL uses standard ERC20 approve.
C4-008 : # Missing zero-address checks in constructor
Impact - LOW
Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are constructed incorrectly.
Proof of Concept
- Navigate to the following contracts.
https://github.com/maple-labs/revenue-distribution-token/blob/main/contracts/RevenueDistributionToken.sol#L48 https://github.com/maple-labs/xMPL/blob/main/contracts/xMPL.sol#L28 https://github.com/maple-labs/mpl-migration/blob/main/contracts/Migrator.sol#L13
First two examples are invalid and third we don't want to add since the migrator is so small it is redeployable
C4-009 : # DOMAIN_SEPARATOR can change
Impact - LOW
The variable DOMAIN_SEPARATOR in contract ERC20Permit is assigned in the constructor and will not change after being initialized. However, if a hard fork happens after the contract deployment, the domain would become invalid on one of the forked chains due to the block.chainid has changed.
Proof of Concept
- Navigate to the following contracts.
https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol#L119
This isn't true, DOMAIN_SEPARATOR is a view function that will change when a fork occurs.
C4-010 : # Current Allowance Is Not Checked
Impact - LOW
During the ERC20 implementation, It has been observed that allowance check is not implemented over the functions. That can cause unintended revert on the functions.
Proof of Concept
- Navigate to the following contracts.
https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol#L65 https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol#L157
Tools Used
Code Review
Recommended Mitigation Steps
Consider implementing allowance check in the related functions. (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol)
function decreaseAllowance(address spender, uint256 subtractedValue) public virtual returns (bool) { address owner = _msgSender(); uint256 currentAllowance = allowance(owner, spender); require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero"); unchecked { _approve(owner, spender, currentAllowance - subtractedValue); } return true; }
If these values are too high an underflow will occur and that is intentional, don't want to spend gas on these checks.
Can ignore
I'm leaving this open and valid. Almost all of these are issues if you forget something, someone else takes over and didn't understand your intention, or you change your mind and forget that the potential issue exists. Leaving it in the audit as QA will at least represent a note to future maintainers.
C4-001 : Incompatibility With Rebasing/Deflationary/Inflationary tokens
Impact - LOW
Maple protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
C4-002 : The Contract Should Approve(0) first
Impact - LOW
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
Proof of Concept
Tools Used
None
Recommended Mitigation Steps
Approve with a zero amount first before setting the actual amount.
C4-003 : 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
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-004 : Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
Impact - LOW
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
Navigate to the following contract.
transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.
Tools Used
Code Review
Recommended Mitigation Steps
Consider using safeTransfer/safeTransferFrom or require() consistently.
C4-005 : Consider making contracts Pausable
Impact - LOW
There are many external risks so my suggestion is that you should consider making the contracts pausable, so in case of an unexpected event, the admin can pause transfers.
Tools Used
Code Review
Recommended Mitigation Steps
Consider making contracts Pausable https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol.
C4-006 : Critical changes should use two-step procedure
Impact - NON CRITICAL
The critical change should be completed with the two step. For instance, migrate function can be implemented with the two step procedure. (accept migrating)
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
C4-007 : # USE SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE
Impact - LOW
Note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
Update to _token.safeApprove(spender, type(uint256).max) in the function.
C4-008 : # Missing zero-address checks in constructor
Impact - LOW
Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are constructed incorrectly.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.
C4-009 : # DOMAIN_SEPARATOR can change
Impact - LOW
The variable DOMAIN_SEPARATOR in contract ERC20Permit is assigned in the constructor and will not change after being initialized. However, if a hard fork happens after the contract deployment, the domain would become invalid on one of the forked chains due to the block.chainid has changed.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
An elegant solution that you may consider applying is from Sushi Trident: https://github.com/sushiswap/trident/blob/concentrated/contracts/pool/concentrated/TridentNFT.sol#L47-L62
C4-010 : # Current Allowance Is Not Checked
Impact - LOW
During the ERC20 implementation, It has been observed that allowance check is not implemented over the functions. That can cause unintended revert on the functions.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Consider implementing allowance check in the related functions. (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol)