DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Low Risk

sma4

Low Risk Issues

Number Issue nstances
[L-01] Use a stricter bound for transferability delay 4
[L-02] Unsafe downcast 2
[L-03] receive()/payable fallback() function does not authorize requests 1
[L-04] External calls in an un-bounded for-loop may result in a DOS 2
[L-05] Using zero as a parameter 4
[L-06] Some tokens may not consider type(uint256).max as an infinite approval 10
[L-07] Multiplication on the result of a division 1
[L-08] Missing checks for address(0x0) in the constructo 10
[L-09] Upgradeable contract uses non-upgradeable version of the OpenZeppelin libraries/contracts 7
[L-10] Constant array index within iteration 6
[L-11] Burn functions should be protected with a modifier 2

[L-01] Use a stricter bound for transferability delay

Description:

file: contracts/libraries/LibOracle.sol
​
72 || timeStamp > block.timestamp || chainlinkPrice <= 0
​
73 || block.timestamp > 2 hours + timeStamp;
​
121 || timeStamp > block.timestamp || chainlinkPrice <= 0 || baseRoundId == 0
​
122 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibOracle.sol#L72

Recommendation: Consider using <= insted of < so that it can cover time up to endStream. block.timestamp <=endStream

[L‑02] Unsafe downcast

When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs

There are 2 instances of this issue:

file: contracts/facets/OwnerFacet.sol
​
55 Asset.assetId = uint8(s.assets.length);
​
281 s.bridge[bridge].vault = uint8(vault);
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/OwnerFacet.sol#L55

[L‑03] receive()/payable fallback() function does not authorize requests

Having no access control on the function (e.g. require(msg.sender == address(weth))) means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds. If the concern is having to spend a small amount of gas to check the sender against an immutable address, the code should at least have a function to rescue mistakenly-sent Ether.

There are 1 instances of this issue:

file: contracts/bridges/BridgeReth.sol
​
37 receive() external payable {}
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/bridges/BridgeReth.sol#L37

[L‑04] External calls in an un-bounded for-loop may result in a DOS

Consider limiting the number of iterations in for-loops that make external calls

There are 2 instances of this issue:

file: contracts/facets/ERC721Facet.sol
​
28 for (uint256 i; i < length;) {
STypes.ShortRecord[] memory shortRecords =
IDiamond(payable(address(this))).getShortRecords(s.assets[i], owner);
for (uint256 j; j < shortRecords.length;) {
if (shortRecords[j].tokenId != 0) {
balance++;
}
unchecked {
j++;
}
}
unchecked {
++i;
}
}
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ERC721Facet.sol#L28-L42

file: contracts/libraries/LibVault.sol
​
47 for (uint256 i; i < bridgeCount;) {
zethTotal += IBridge(bridges[i]).getZethValue();
unchecked {
++i;
}
}
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibVault.sol#L47-L52

[L-05] Using zero as a parameter

Taking 0 as a valid argument in Solidity without checks can lead to severe security issues. A historical example is the infamous 0x0 address bug where numerous tokens were lost. This happens because 0 can be interpreted as an uninitialized address, leading to transfers to the 0x0 address, effectively burning tokens. Moreover, 0 as a denominator in division operations would cause a runtime exception. It's also often indicative of a logical error in the caller's code. It's important to always validate input and handle edge cases like 0 appropriately. Use require() statements to enforce conditions and provide clear error messages to facilitate debugging and safer code.

file: contracts/facets/OwnerFacet.sol
​
91 LibShortRecord.createShortRecord(
asset, address(this), SR.FullyFilled, 0, 0, 0, 0, 0
);
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/OwnerFacet.sol#L91

file: contracts/facets/BidOrdersFacet.sol
​
152 return (0, ercAmount);
​
337 return (0, incomingBid.ercAmount);
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/BidOrdersFacet.sol#L152

file: contracts/facets/MarginCallPrimaryFacet.sol
​
121 return (0, 0);
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/MarginCallPrimaryFacet.sol#L121

[L‑06] Some tokens may not consider type(uint256).max as an infinite approval

Some tokens such as COMP downcast such approvals to uint96 and use that as a raw value rather than interpreting it as an infinite approval. Eventually these approvals will reach zero, at which point the calling contract will no longer function properly.

There are 10 instances of this issue:

file: contracts/facets/ERC721Facet.sol
​
143 if (tokenId > type(uint40).max) revert Errors.InvalidTokenId();
​
197 if (tokenId > type(uint40).max) revert Errors.InvalidTokenId();
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ERC721Facet.sol#L143

file: contracts/facets/MarginCallPrimaryFacet.sol
​
283 if (a > type(uint88).max) revert Errors.InvalidAmount();
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/MarginCallPrimaryFacet.sol#L283

file: contracts/facets/MarginCallSecondaryFacet.sol
​
191 if (a > type(uint88).max) revert Errors.InvalidAmount();
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/MarginCallSecondaryFacet.sol#L191

file: contracts/facets/YieldFacet.sol
​
141 if (dittoReward > type(uint80).max) revert Errors.InvalidAmount();
​
174 if ((totalReward - userReward) > type(uint96).max) revert Errors.InvalidAmount();
​
177 if (userReward > type(uint80).max) revert Errors.InvalidAmount();

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/YieldFacet.sol#L141

file: contracts/libraries/LibShortRecord.sol
​
84 if (tokenId > type(uint40).max) revert Errors.InvalidTokenId();
​
269 if (id < type(uint8).max) {
​
397 } else if (s.flaggerIdCounter < type(uint16).max) {
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibShortRecord.sol#L84

[L‑07] Multiplication on the result of a division

Dividing an integer by another integer will often result in loss of precision. When the result is multiplied by another number, the loss of precision is magnified, often to material magnitudes. (X / Z) * Y should be re-written as (X * Y) / Z

There are 1 instances of this issue:

file: contracts/libraries/LibOracle.sol
​
85 uint256 twapPriceInEther = (twapPrice / Constants.DECIMAL_USDC) * 1 ether;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibOracle.sol#L85

[L‑08] Missing checks for address(0x0) in the constructor

There are 10 instances of this issue:

file: contracts/bridges/BridgeReth.sol
​
22 diamond = diamondAddr;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/bridges/BridgeReth.sol

file: contracts/bridges/BridgeSteth.sol
​
24 diamond = diamondAddr;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/bridges/BridgeSteth.sol#L24

file: contracts/facets/BridgeRouterFacet.sol
​
28 rethBridge = _rethBridge;
​
29 stethBridge = _stethBridge;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/BridgeRouterFacet.sol#L28

file: contracts/facets/ExitShortFacet.sol
​
30 cusd = _cusd;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ExitShortFacet.sol#L30

file: contracts/facets/MarginCallPrimaryFacet.sol
​
30 cusd = _cusd;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/MarginCallPrimaryFacet.sol#L30

file: contracts/facets/ShortRecordFacet.sol
​
26 cusd = _cusd;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ShortRecordFacet.sol#L26

file: contracts/facets/VaultFacet.sol
​
27 carbonZeth = _zeth;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/VaultFacet.sol#L27

file: contracts/tokens/Asset.sol
​
14 diamond = diamondAddr;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/tokens/Asset.sol#L14

file: contracts/tokens/Ditto.sol
16 diamond = diamondAddr;
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/tokens/Ditto.sol#L16

[L‑09] Upgradeable contract uses non-upgradeable version of the OpenZeppelin libraries/contracts

OpenZeppelin has an Upgradeable variants of each of its libraries and contracts, and upgradeable contracts should use those variants.

There is 7 instance of this issue:

file: contracts/bridges/BridgeSteth.sol
​
8 import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/bridges/BridgeSteth.sol#L8

file: contracts/facets/ERC721Facet.sol
​
4 import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
​
5 import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ERC721Facet.sol#L4

file: contracts/libraries/LibOracle.sol
​
8 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibOracle.sol#L8

file: contracts/tokens/Asset.sol
​
4 import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/tokens/Asset.sol#L4

file: contracts/tokens/Ditto.sol
​
7 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";
​
8 import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/tokens/Ditto.sol#L7

[L-10] Constant array index within iteration

Using constant array indexes within for loops is error prone as typically [i] is used instead as by using a constant index such as [0] means that only one value will be used/modified within an array, historically many bugs have been introduced through using a constant index rather than the array iteration index i.e i,j. Provided this is intentional and not a typo consider using enum values to make this distinction clear.

file: contracts/bridges/BridgeSteth.sol
​
93 amountArray[0] = amount;
​
94 uint256 requestId = unsteth.requestWithdrawals(amountArray, address(this))[0];

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/bridges/BridgeSteth.sol#L93

file: contracts/facets/ShortRecordFacet.sol
​
121 onlyValidShortRecord(asset, msg.sender, ids[0])
​
125 STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ShortRecordFacet.sol#L121

file: contracts/facets/YieldFacet.sol
​
55 uint256 vault = s.asset[assets[0]].vault;
​
58 (uint88 yield, uint256 dittoYieldShares) = _distributeYield(assets[0]);
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/YieldFacet.sol#L55

[L-11] Burn functions should be protected with a modifier

file: contracts/libraries/LibShortRecord.sol
​
​
363 function burnNFT(uint256 tokenId) internal {
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibShortRecord.sol#L363

file: contracts/libraries/LibAsset.sol
​
11 function burnMsgSenderDebt(address asset, uint88 debt) internal {
​

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/LibAsset.sol#L11

Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.