XDEFI contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

Waste of gas due to unnecessary underflow checks

Proof of Concept

On XDEFIDistribution.sol:120 and XDEFIDistribution.sol:175, you can find the following substraction: uint256 withdrawAmount = amountUnlocked_ - lockAmount_;

However, as the Solidity version is 0.8.10, default overflow and underflow checks are made, which cost some gas.

You can save this gas with the unchecked keyword to bypass these checks as 5 lines above (L115 and L170), a require statement already checks that lockAmount_ <= amountUnlocked_.

Therefore, no underflow is possible.

Tools Used

VS Code

Recommended Mitigation Steps

Use the "unchecked" keyword

Handle

Dravee

Vulnerability details

Impact

The compiler does not reserve a storage slot for constant variables. Therefore, a state variable that is never changed should be declared constant to save gas.

Proof of Concept

Instances include:

XDEFIDistribution.sol:14:    
uint88 internal MAX_TOTAL_XDEFI_SUPPLY = uint88(240_000_000_000_000_000_000_000_000);

Tools Used

Slither (Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant)

Recommended Mitigation Steps

Add the constant attribute to state variables that never change.

Handle

Dravee

Vulnerability details

Impact

Public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

This function is never called by the contract:

XDEFIDistribution.withdrawableOf(uint256) (contracts/XDEFIDistribution.sol#156-159)

Tools Used

Slither

Recommended Mitigation Steps

Change function visibility from public to external

Handle

Dravee

Vulnerability details

Impact

On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory :

memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Proof of Concept

interfaces\IXDEFIDistribution.sol:55:    function baseURI() external view returns (string memory baseURI_);
interfaces\IXDEFIDistribution.sol:74:    function setBaseURI(string memory baseURI_) external;
interfaces\IXDEFIDistribution.sol:77:    function setLockPeriods(uint256[] memory durations_, uint8[] memory multipliers) external;
interfaces\IXDEFIDistribution.sol:106:    function relockBatch(uint256[] memory tokenIds_, uint256 lockAmount_, uint256 duration_, address destination_) external returns (uint256 amountUnlocked_, uint256 
newTokenId_);
interfaces\IXDEFIDistribution.sol:109:    function unlockBatch(uint256[] memory tokenIds_, address destination_) external returns (uint256 amountUnlocked_);
interfaces\IXDEFIDistribution.sol:119:    function merge(uint256[] memory tokenIds_, address destination_) external returns (uint256 tokenId_);
interfaces\IXDEFIDistribution.sol:125:    function tokenURI(uint256 tokenId_) external view returns (string memory tokenURI_);
XDEFIDistribution.sol:73:    function setBaseURI(string memory baseURI_) external onlyOwner {
XDEFIDistribution.sol:77:    function setLockPeriods(uint256[] memory durations_, uint8[] memory multipliers) external onlyOwner {
XDEFIDistribution.sol:165:    function relockBatch(uint256[] memory tokenIds_, uint256 lockAmount_, uint256 duration_, address destination_) external noReenter returns (uint256 amountUnlocked_, uint256 
newTokenId_) {
XDEFIDistribution.sol:186:    function unlockBatch(uint256[] memory tokenIds_, address destination_) external noReenter returns (uint256 amountUnlocked_) {
XDEFIDistribution.sol:205:    function merge(uint256[] memory tokenIds_, address destination_) external returns (uint256 tokenId_) {

Tools Used

VS Code

Recommended Mitigation Steps

Use calldata instead of memory for external functions where the function argument is read-only.

Handle

Dravee

Vulnerability details

Impact

!= 0 costs less gas compared to > 0 for unsigned integer

Proof of Concept

> 0 is used in the following location(s):

XDEFIDistribution.sol:144:        require(totalUnitsCached > uint256(0), "NO_UNIT_SUPPLY");

Tools Used

VS Code

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

Dravee

Vulnerability details

Impact

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save 77 gas per iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Proof of Concept

Instances include:

XDEFIDistribution.sol:80:        for (uint256 i; i < count; ++i) {
XDEFIDistribution.sol:212:        for (uint256 i; i < count; ++i) {
XDEFIDistribution.sol:325:        for (uint256 i; i < count; ++i) {

Tools Used

VS Code

Recommended Mitigation Steps

The code would go from:

for (uint256 i; i < numIterations; ++i) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

While the risk are overflow is inexistant for a uint256 i, you might want to keep in mind to manually check for bounds once before the for-loop if i is smaller than a uint256 (such as uint8, but it's never recommended and it's not the case in this project).

Handle

Dravee

Vulnerability details

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings.

Proof of Concept

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

XDEFIDistribution.sol:40:        require((XDEFI = XDEFI_) != address(0), "INVALID_TOKEN");
XDEFIDistribution.sol:47:        require(owner == msg.sender, "NOT_OWNER");
XDEFIDistribution.sol:52:        require(_locked == 0, "LOCKED");
XDEFIDistribution.sol:63:        require(pendingOwner == msg.sender, "NOT_PENDING_OWNER");
XDEFIDistribution.sol:82:            require(duration <= uint256(18250 days), "INVALID_DURATION");
XDEFIDistribution.sol:115:        require(lockAmount_ <= amountUnlocked_, "INSUFFICIENT_AMOUNT_UNLOCKED");
XDEFIDistribution.sol:145:        require(totalUnitsCached > uint256(0), "NO_UNIT_SUPPLY");
XDEFIDistribution.sol:170:        require(lockAmount_ <= amountUnlocked_, "INSUFFICIENT_AMOUNT_UNLOCKED");
XDEFIDistribution.sol:207:        require(count > uint256(1), "MIN_2_TO_MERGE");
XDEFIDistribution.sol:214:            require(ownerOf(tokenId) == msg.sender, "NOT_OWNER");
XDEFIDistribution.sol:215:            require(positionOf[tokenId].expiry == uint32(0), "POSITION_NOT_UNLOCKED"); 
XDEFIDistribution.sol:227:        require(_exists(tokenId_), "NO_TOKEN");
XDEFIDistribution.sol:232:        require(_exists(tokenId_), "NO_TOKEN");
XDEFIDistribution.sol:255:        require(amount_ != uint256(0) && amount_ <= MAX_TOTAL_XDEFI_SUPPLY, "INVALID_AMOUNT");
XDEFIDistribution.sol:259:        require(bonusMultiplier != uint8(0), "INVALID_DURATION");
XDEFIDistribution.sol:295:        require(ownerOf(tokenId_) == account_, "NOT_OWNER");
XDEFIDistribution.sol:304:        require(expiry != uint32(0), "NO_LOCKED_POSITION");
XDEFIDistribution.sol:305:        require(block.timestamp >= uint256(expiry), "CANNOT_UNLOCK");
XDEFIDistribution.sol:322:        require(count > uint256(1), "USE_UNLOCK");

Tools Used

VS Code

Recommended Mitigation Steps

Replace revert strings with custom errors.

Handle

Dravee

Vulnerability details

Impact

For SSTORE: the current cost per one 32-byte word is 20,000 Gas For SLOAD, it's 800 gas.

Proof of Concept

In the XDEFIDistribution.sol contract, here's the proposeOwnership function:

    function proposeOwnership(address newOwner_) external onlyOwner {
        emit OwnershipProposed(owner, pendingOwner = newOwner_); 
    }

A require statement should be there to avoid a SSTORE, such as if pendingOwner is already the newOwner or if pendingOwner is the 0 address.

However, this check would cost at least a SLOAD and an MLOAD.

Tools Used

VS Code

Recommended Mitigation Steps

Applying the check is all about estimating the risk-cost of a mistake in the proposeOwnership function call. As it's an external function and there's a 1/20 ratio between the SLOAD cost and the SSTORE cost, I'd recommend applying the check if you think there's a >5% chance of calling the method with the currently pending owner as a parameter by mistake

Handle

Dravee

Vulnerability details

Impact

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

Proof of Concept

In XDEFIDistribution.sol, the order of variables is this way:

    uint88 internal MAX_TOTAL_XDEFI_SUPPLY = uint88(240_000_000_000_000_000_000_000_000);
    uint256 internal constant _pointsMultiplier = uint256(2**128);
    uint256 internal _pointsPerUnit;
    address public immutable XDEFI;
    uint256 public distributableXDEFI;
    uint256 public totalDepositedXDEFI;
    uint256 public totalUnits;
    mapping(uint256 => Position) public positionOf;
    mapping(uint256 => uint8) public bonusMultiplierOf;  // Scaled by 100 (i.e. 1.1x is 110, 2.55x is 255).
    uint256 internal immutable _zeroDurationPointBase;
    string public baseURI;
    address public owner;
    address public pendingOwner;
    uint256 internal _locked;

As we can see, the first variable is a uint88, which is a 12 bytes size variable (way less than 32 bytes). However, due to it's wrong position, it will take up a whole 32 bytes slot. As an address type variable is of size 20 bytes, it would be better to make them contiguous so that they are packed together in a 32 bytes slot.

As for the string variable, it's a dynamic size variable, so it'll always take up a new slot

Recommended Mitigation Steps

Pack the variables in XDEFIDistribution.sol such that the number of slots used in deployment is minimised. The only optimization I can see here is moving the uint88 variable (12 bytes) next to an address variable (20 bytes).