code-423n4 / 2022-01-xdefi-findings

0 stars 0 forks source link

Adding unchecked directive can save gas #125

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L110-L120

function relock(uint256 tokenId_, uint256 lockAmount_, uint256 duration_, address destination_) external noReenter returns (uint256 amountUnlocked_, uint256 newTokenId_) {
        // Handle the unlock and get the amount of XDEFI eligible to withdraw.
        amountUnlocked_ = _unlock(msg.sender, tokenId_);

        // Throw convenient error if trying to re-lock more than was unlocked. `amountUnlocked_ - lockAmount_` would have reverted below anyway.
        require(lockAmount_ <= amountUnlocked_, "INSUFFICIENT_AMOUNT_UNLOCKED");

        // Handle the lock position creation and get the tokenId of the locked position.
        newTokenId_ = _lock(lockAmount_, duration_, destination_);

        uint256 withdrawAmount = amountUnlocked_ - lockAmount_;

amountUnlocked_ - lockAmount_ can be unchecked as L115 ensured lockAmount_ <= amountUnlocked_ so that it will never underflow.

Recommendation

Change to:

unchecked {
    uint256 withdrawAmount = amountUnlocked_ - lockAmount_;
}        
deluca-mike commented 2 years ago

Agreed. We originally abstained from unchecked math, for cleaner code, but we will now use unchecked wherever possible.

deluca-mike commented 2 years ago

Duplicate #49