Decent - 0x11singh99's results

Decent enables one-click transactions using any token across chains.

General Information

Platform: Code4rena

Start Date: 19/01/2024

Pot Size: $36,500 USDC

Total HM: 9

Participants: 113

Period: 3 days

Judge: 0xsomeone

Id: 322

League: ETH

Decent

Findings Distribution

Researcher Performance

Rank: 26/113

Findings: 2

Award: $192.08

🌟 Selected for report: 0

🚀 Solo Findings: 0

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DcntEth.sol#L20-L30

Vulnerability details

Summary

DcntEth::setRouter function has no access control. In this function router state variable is set. After that router is responsible to mint and burn DcntEth tokens by calling DcntEth::mint and DcntEth::burn functions. Since anyone can set router using setRouter function so attacker can call setRouter function by passing his address and can become router. When attacker's address will be set as router,then he can call mint or burn functions of DcntEth.sol,since these functions are protected by onlyRouter modifier and attacker have become router already using setRouter. So he can mint/burn his desired quantity of DcntEth tokens whatever he want.

Vulnerability Details

We can see in below code snippet there is no access control on setRouter function. So anyone can call this and set his address into router state variable. After becoming router he can call mint and burn easily with his desired quantity or unlimited quantity of DcntEth tokens can be minted to attacker's passed address.

src/DcntEth.sol#L20-L30

        //@audit-issue Missing access control
20:    function setRouter(address _router) public {
21:        router = _router;
22:    }
       //@audit attacker can call this function by becoming router
24:    function mint(address _to, uint256 _amount) public onlyRouter {
25:        _mint(_to, _amount);
       }
       //@audit attacker can call this function by becoming router
28:    function burn(address _from, uint256 _amount) public onlyRouter {
29:        _burn(_from, _amount);
30:    }

onlyRouter Modifier src/DcntEth.sol#L8-L11

8:  modifier onlyRouter() {
9:       require(msg.sender == router);
10:       _;
11:   }

Impact

Attacker can become router by passing his address in setRouter function. And he can mint unlimited DcntEth tokens to himself Or he can burn all the DcntEth tokens. By this the purpose of DcntEth tokens will be totally destroyed. Attacker can take over all DcntEth tokens.

Tools Used

Manual Review

Use any access control in the setRouter function. So only allowed role can call this crucial function not everyone. Consider adding onlyOwner modifier in this function since it is already used in other functions of this file mintByOwner and burnByOwner.


- 20:    function setRouter(address _router) public {
+ 20:    function setRouter(address _router) public onlyOwner {
  21:        router = _router;
  22:    }

Assessed type

Access Control

#0 - c4-pre-sort

2024-01-25T03:08:07Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-01-25T03:08:14Z

raymondfam marked the issue as duplicate of #14

#2 - c4-judge

2024-02-03T13:10:26Z

alex-ppg marked the issue as satisfactory

Findings Information

🌟 Selected for report: c3phas

Also found by: 0x11singh99, Raihan, dharma09, hunter_w3b, slvDev

Labels

bug
G (Gas Optimization)
grade-a
sufficient quality report
G-01

Awards

191.9635 USDC - $191.96

External Links

Gas Optimizations

Note: It includes only those instances which were Missed by bot-report.

[G-01] State variables only set in the constructor should be declared immutable(Missed by bot)

Note: It includes only those instances which were Missed by bot-report

Accessing state variables within a function involves an SLOAD operation, but immutable variables can be accessed directly without the need of it, thus reducing gas costs. As these state variables are assigned only in the constructor, consider declaring them immutable. Avoids a Gsset(** 20000 gas**) in the constructor, and replaces the first access in each transaction(Gcoldsload - ** 2100 gas **) and each access thereafter(Gwarmacces - ** 100 gas ) with aPUSH32( 3 gas **).

4 instances

Total Gas Saved ~8000

File : src/DecentBridgeExecutor.sol

09:     IWETH weth;
10:     bool public gasCurrencyIsEth;

09-10

File : src/bridge_adapters/DecentBridgeAdapter.sol

19:     address bridgeToken;

19

File : src/DecentEthRouter.sol

15:    IDecentBridgeExecutor public executor;

15

[G-02] Set wrapped variable address in constructor instead of function and make it immutable

wrapped is referring to ERC20 version of native currency of that chain. like WETH for Ethereum Mainnet. So they are one per chain and fixed contracts on every chain. So they can be marked immutable since these contracts using them will be deployed on different chains separately.

Avoids a Gsset(20000 gas) in the constructor, and replaces the first access in each transaction(Gcoldsload - 2100 gas) and each access thereafter(Gwarmacces - 100 gas ) with aPUSH32( 3 gas)

2 instances

Total Gas Saved ~120,000 Deployments Gas and Gsset(20000 gas) in the constructor Gcoldsload - 2100 gas.

Set address wrapped in the constructor and make it immutable and remove the setter function setWrapped

File : src/swappers/UniSwapper.sol

    constructor() UTBOwned() {}
...
    address payable public wrapped;

...
    function setWrapped(address payable _wrapped) public onlyOwner {
        wrapped = _wrapped;
    }

14-26

File : src/swappers/UniSwapper.sol

-   constructor() UTBOwned() {}

    uint8 public constant SWAPPER_ID = 0;
    address public uniswap_router;
-   address payable public wrapped;

+   address payable public immutable wrapped;

+   constructor(address payable _wrapped) UTBOwned() {
+       wrapped = _wrapped; 
+ }

    function setRouter(address _router) public onlyOwner {
        uniswap_router = _router;
    }

-    function setWrapped(address payable _wrapped) public onlyOwner {
-        wrapped = _wrapped;
-    }

Set wrapped in the constructor and make it immutable and remove the setter function setWrapped

File : src/UTB.sol

     constructor() Owned(msg.sender) {}
...
     IWETH wrapped;
  
...
     function setWrapped(address payable _wrapped) public onlyOwner {
        wrapped = IWETH(_wrapped);
    }

16-38

File : src/UTB.sol

-   constructor() Owned(msg.sender) {}

    IUTBExecutor executor;
    IUTBFeeCollector feeCollector;
-   IWETH wrapped;
+   IWETH immutable wrapped;
    mapping(uint8 => address) public swappers;
    mapping(uint8 => address) public bridgeAdapters;

+   constructor(address payable _wrapped) Owned(msg.sender) {
+        wrapped = IWETH(_wrapped);
+   }


    /**
     * @dev Sets the executor.
     * @param _executor The address of the executor.
     */
    function setExecutor(address _executor) public onlyOwner {
        executor = IUTBExecutor(_executor);
    }

    /**
     * @dev Sets the wrapped native token.
     * @param _wrapped The address of the wrapped token.
     */
-    function setWrapped(address payable _wrapped) public onlyOwner {
-        wrapped = IWETH(_wrapped);
-    }

[G-03] Remove unnecessary stack variable creation

If you want to avoid magic numbers then use constant

1 instance

Total Gas Saved ~10

File : src/DecentEthRouter.sol

96:        uint256 GAS_FOR_RELAY = 100000;
97:        uint256 gasAmount = GAS_FOR_RELAY + _dstGasForCall;

96-97

[G-04] Remove unnecessary modifier

2 instances

Total Gas Saved ~60000

Unnecessary onlyIfWeHaveEnoughReserves(amount) modifier check

onlyIfWeHaveEnoughReserves modifier is checking that our DecentEthRouter contract have enough weth or not. Since It will be checked in Weth contract also when this DecentEthRouter will call withdraw or transfer function in weth contract. And when this caller contract don't have enough reserves WETH contract will revert automatically which will revert entire transaction. So there is no need to add extra modifier check here in our contract checking same thing. It will save gas for not using that modifier and deleting this modifier will also some save deployment gas also.

Saves ~30000 GAS

It will save the gas every time redeemEth function called.

File : src/DecentEthRouter.sol

285:    function redeemEth(
286:        uint256 amount
287:    ) public onlyIfWeHaveEnoughReserves(amount) {
288:        dcntEth.transferFrom(msg.sender, address(this), amount);
289:        weth.withdraw(amount);
290:        payable(msg.sender).transfer(amount);
291:    }

285-291

File : src/DecentEthRouter.sol

        function redeemEth(
        uint256 amount
-     ) public onlyIfWeHaveEnoughReserves(amount) {
+     ) public {
+       weth.withdraw(amount);
        dcntEth.transferFrom(msg.sender, address(this), amount);
-       weth.withdraw(amount);
        payable(msg.sender).transfer(amount);
    }

Saves ~30000 GAS

It will save the gas every time redeemWeth function called.

File : src/DecentEthRouter.sol

294:    function redeemWeth(
295:        uint256 amount
296:    ) public onlyIfWeHaveEnoughReserves(amount) {
297:        dcntEth.transferFrom(msg.sender, address(this), amount);
298:        weth.transfer(msg.sender, amount);
299:    }

294-299

File : src/DecentEthRouter.sol

    function redeemWeth(
     uint256 amount
-    ) public onlyIfWeHaveEnoughReserves(amount) {
+    ) public {
+       weth.transfer(msg.sender, amount);
        dcntEth.transferFrom(msg.sender, address(this), amount);
-       weth.transfer(msg.sender, amount);
    }

[G-05] Function should be mark payable to save gas

Note: It includes only those instances which were Missed by bot-report

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

6 instances

Total Gas Saved ~24*6 => ~144 GAS

File : src/DcntEth.sol

24:    function mint(address _to, uint256 _amount) public onlyRouter {    

28:    function burn(address _from, uint256 _amount) public onlyRouter {

32:    function mintByOwner(address _to, uint256 _amount) public onlyOwner {

36:    function burnByOwner(address _from, uint256 _amount) public onlyOwner {

24, 28, 32, 36

File : src/UTBFeeCollector.sol

18:   function setSigner(address _signer) public onlyOwner {

69:   function redeemFees(address token, uint amount) public onlyOwner {   

18, 69

[G-06] Don't cache value if it is used only once

If a value is only intended to be used once then it should not be cached. Caching the value will result in unnecessary stack manipulation.

2 instances

Total Gas Saved ~20 Gas

Don't cache ISwapper(swapper) and IBridgeAdapter(bridge)

File : src/UTB.sol

326:     ISwapper s = ISwapper(swapper);
327:       swappers[s.getId()] = swapper;

335:     IBridgeAdapter b = IBridgeAdapter(bridge);
336:        bridgeAdapters[b.getId()] = bridge;

326-327, 335-336

File : src/UTB.sol

-       ISwapper s = ISwapper(swapper);
-        swappers[s.getId()] = swapper;
+        swappers[ISwapper(swapper).getId()] = swapper;

-      IBridgeAdapter b = IBridgeAdapter(bridge);
-        bridgeAdapters[b.getId()] = bridge;
+        bridgeAdapters[IBridgeAdapter(bridge).getId()] = bridge;

[G-07] Use calldata instead of memory

Note: Missed by bot-report

When you specify a data location as memory, that value will be copied into memory. When you specify the location as calldata, the value will stay static within calldata. If the value is a large, complex type, using memory may result in extra memory expansion costs.

5 instances

Total Gas Saved ~ 476*5 = ~ 2380 Gas

File : src/DecentEthRouter.sol

113:    function estimateSendAndCallFee(
114:         uint8 msgType,
115:         uint16 _dstChainId,
116:         address _toAddress,
117:         uint _amount,
118:         uint64 _dstGasForCall,
119:         bool deliverEth,
120:         bytes memory payload //@audit use calldata
121:     ) public view returns (uint nativeFee, uint zroFee) {


197:    function bridgeWithPayload(
198:         uint16 _dstChainId,
199:         address _toAddress,
200:         uint _amount,
201:         bool deliverEth,
202:         uint64 _dstGasForCall,
203:         bytes memory additionalPayload //@audit use calldata
204:     ) public payable {


237:     function onOFTReceived(
238:         uint16 _srcChainId,
239:         bytes calldata,
240:         uint64,
241:         bytes32,
242:         uint _amount,
243:         bytes memory _payload //@audit use calldata
244:     ) external override onlyLzApp {  

113-121, 197-204, 237-244

File : src/UTBFeeCollector.sol

44:     function collectFees(
45:          FeeStructure calldata fees,
46:          bytes memory packedInfo, //@audit use calldata
47:          bytes memory signature //@audit use calldata
48:      ) public payable onlyUtb {

44-48

[G-08] Remove unnecessary external pure functions

Since these are external pure functions so neither they can be used in this contract nor they telling something about contract. So it is unnecessary to have external pure functions in contract purely for calculation purpose. If it is called by another contract than it will be better they do it in their contract. If it is called from outside then it will be better to do it there. Since they are not serving any purpose related to smart contract so they can be removed.

2 instances

File : src/bridge_adapters/StargateBridgeAdapter.sol

55:    function getBridgeToken(

61:    function getBridgedAmount(        

55, 61

[G-09] Remove unused import

Note: It includes only those instances which were Missed by bot-report

2 instances

File : src/UTBFeeCollector.sol

      //@audit `SwapInstructions` and `BridgeInstructions`
05:   import {SwapInstructions, FeeStructure, BridgeInstructions} from "./CommonTypes.sol";

05

[G-10] Check amount for zero value

Note: It includes only those instances which were Missed by bot-report

6 instances

File : src/DcntEth.sol

24:    function mint(address _to, uint256 _amount) public onlyRouter {
25:          _mint(_to, _amount); //@audit check `_amount` for zero
26:     }
27:
28:    function burn(address _from, uint256 _amount) public onlyRouter {
29:         _burn(_from, _amount); //@audit check `_amount` for zero
30:     }
31:
32:    function mintByOwner(address _to, uint256 _amount) public onlyOwner {
33:         _mint(_to, _amount); //@audit check `_amount` for zero
34:    }
35:
36:    function burnByOwner(address _from, uint256 _amount) public onlyOwner {
37:         _burn(_from, _amount); //@audit check `_amount` for zero
38:    }

24-38

File : src/UTBFeeCollector.sol

70:     if (token == address(0)) {
71:            payable(owner).transfer(amount); //@audit check `amount` for zero
72:     } else {
73:            IERC20(token).transfer(owner, amount); //@audit check `amount` for zero
74:        }

70-74

[G-11] If any operation can't be underflow/overflow should be used unchecked

Note: It includes only those instances which were Missed by bot-report

2 instances

File : src/DecentEthRouter.sol

61:        uint256 balance = balanceOf[msg.sender];
62:        require(balance >= amount, "not enough balance");
63:        _;
64:        balanceOf[msg.sender] -= amount;
65:    }

61-65

File : src/DecentEthRouter.sol

          //@audit can be marked unchecked since first is `100000` and second is uint64 so no chance to overflow
97:       uint256 gasAmount = GAS_FOR_RELAY + _dstGasForCall;

97

[G-12] Use already cached value

2 instances

File : src/DecentEthRouter.sol

61:        uint256 balance = balanceOf[msg.sender];
62:        require(balance >= amount, "not enough balance");
63:        _;
64:        balanceOf[msg.sender] -= amount;
65:    }

61-65

File : src/DecentEthRouter.sol

         uint256 balance = balanceOf[msg.sender];
         require(balance >= amount, "not enough balance");
         _;
-        balanceOf[msg.sender] -= amount;
+        balanceOf[msg.sender] = balance - amount;
    }

[G-13] Cache state variable rather than re-reading from storage

Note: It includes only those instances which were Missed by bot-report

6 instances

Cache weth

File : src/DecentBridgeExecutor.sol

30:        uint256 balanceBefore = weth.balanceOf(address(this));
31:        weth.approve(target, amount);
32:
33:        (bool success, ) = target.call(callPayload);
34:
35:        if (!success) {
36:            weth.transfer(from, amount);
37:            return;
38:        }
39:
40:        uint256 remainingAfterCall = amount -
41:            (balanceBefore - weth.balanceOf(address(this)));
42:
43:        // refund the sender with excess WETH
44:        weth.transfer(from, remainingAfterCall);

30-44

File : src/DecentBridgeExecutor.sol

+       IWETH _weth = weth;
-       uint256 balanceBefore = weth.balanceOf(address(this));
-       weth.approve(target, amount);
+       uint256 balanceBefore = _weth.balanceOf(address(this));
+       _weth.approve(target, amount);

        (bool success, ) = target.call(callPayload);

        if (!success) {
-            weth.transfer(from, amount);
+            _weth.transfer(from, amount);
             return;
        }

        uint256 remainingAfterCall = amount -
-            (balanceBefore - weth.balanceOf(address(this)));
+            (balanceBefore - _weth.balanceOf(address(this)));

        // refund the sender with excess WETH
-        weth.transfer(from, remainingAfterCall);
+        _weth.transfer(from, remainingAfterCall);

Cache weth

File : src/DecentEthRouter.sol

266:        if (weth.balanceOf(address(this)) < _amount) {
267:            dcntEth.transfer(_to, _amount);
268:            return;
269:        }
270:
271:        if (msgType == MT_ETH_TRANSFER) {
272:            if (!gasCurrencyIsEth || !deliverEth) {
273:                weth.transfer(_to, _amount);
274:            } else {
275:                weth.withdraw(_amount);
276:                payable(_to).transfer(_amount);
277:            }
278:        } else {
289:            weth.approve(address(executor), _amount);

266-279

File : src/DecentEthRouter.sol

+       IWETH _weth = weth;
-       if (weth.balanceOf(address(this)) < _amount) {
+       if (_weth.balanceOf(address(this)) < _amount) {
            dcntEth.transfer(_to, _amount);
            return;
        }

        if (msgType == MT_ETH_TRANSFER) {
            if (!gasCurrencyIsEth || !deliverEth) {
-               weth.transfer(_to, _amount);
+               _weth.transfer(_to, _amount);
            } else {
-               weth.withdraw(_amount);
+               _weth.withdraw(_amount);
                payable(_to).transfer(_amount);
            }
        } else {
-            weth.approve(address(executor), _amount);
+            _weth.approve(address(executor), _amount);

Cache executor

File : src/DecentEthRouter.sol

279:    weth.approve(address(executor), _amount);
280:    executor.execute(_from, _to, deliverEth, _amount, callPayload);

279-280

File : src/DecentEthRouter.sol

+       IDecentBridgeExecutor _executor = executor;
-       weth.approve(address(executor), _amount);
-       executor.execute(_from, _to, deliverEth, _amount, callPayload);
+       weth.approve(address(_executor), _amount);
+       _executor.execute(_from, _to, deliverEth, _amount, callPayload);

Cache wrapped

File : src/UTB.sol

74:     if (swapParams.tokenIn == address(0)) {
75:            require(msg.value >= swapParams.amountIn, "not enough native");
76:            wrapped.deposit{value: swapParams.amountIn}();
77:            swapParams.tokenIn = address(wrapped);
78:            swapInstructions.swapPayload = swapper.updateSwapParams(
79:                swapParams,
80:                swapInstructions.swapPayload
81:            );
82:        } else if (retrieveTokenIn) {
83:            IERC20(swapParams.tokenIn).transferFrom(
84:                msg.sender,
85:                address(this),
86:                swapParams.amountIn
87:            );
88:        }
89:
90:        IERC20(swapParams.tokenIn).approve(
91:            address(swapper),
92:            swapParams.amountIn
93:        );
94:
95:        (tokenOut, amountOut) = swapper.swap(swapInstructions.swapPayload);
96:
97:        if (tokenOut == address(0)) {
98:            wrapped.withdraw(amountOut);

74-98

File : src/UTB.sol

+       IWETH _wrapped = wrapped;
        if (swapParams.tokenIn == address(0)) {
            require(msg.value >= swapParams.amountIn, "not enough native");
-           wrapped.deposit{value: swapParams.amountIn}();
-           swapParams.tokenIn = address(wrapped);
+           _wrapped.deposit{value: swapParams.amountIn}();
+           swapParams.tokenIn = address(_wrapped);
            swapInstructions.swapPayload = swapper.updateSwapParams(
                swapParams,
                swapInstructions.swapPayload
            );
        } else if (retrieveTokenIn) {
            IERC20(swapParams.tokenIn).transferFrom(
                msg.sender,
                address(this),
                swapParams.amountIn
            );
        }

        IERC20(swapParams.tokenIn).approve(
            address(swapper),
            swapParams.amountIn
        );

        (tokenOut, amountOut) = swapper.swap(swapInstructions.swapPayload);

        if (tokenOut == address(0)) {
-            wrapped.withdraw(amountOut);
+            _wrapped.withdraw(amountOut);

Cache executor

File : src/UTB.sol

152:    IERC20(tokenOut).approve(address(executor), amountOut);
153:            executor.execute(

152-153

File : src/UTB.sol

+       IUTBExecutor _executor = executor;
-        IERC20(tokenOut).approve(address(executor), amountOut);
-            executor.execute(
+        IERC20(tokenOut).approve(address(_executor), amountOut);
+            _executor.execute(

Cache feeCollector

File : src/UTB.sol

233:    if (address(feeCollector) != address(0)) {
234:            uint value = 0;
235:            if (fees.feeToken != address(0)) {
236:                IERC20(fees.feeToken).transferFrom(
237:                    msg.sender,
238:                    address(this),
239:                    fees.feeAmount
240:                );
241:                IERC20(fees.feeToken).approve(
242:                    address(feeCollector),
243:                    fees.feeAmount
244:                );
245:            } else {
246:                value = fees.feeAmount;
247:            }
248:            feeCollector.collectFees{value: value}(fees, packedInfo, signature);

233-248

File : src/UTB.sol

+       IUTBFeeCollector _feeCollector = feeCollector;
-        if (address(feeCollector) != address(0)) {
+        if (address(_feeCollector) != address(0)) {
            uint value = 0;
            if (fees.feeToken != address(0)) {
                IERC20(fees.feeToken).transferFrom(
                    msg.sender,
                    address(this),
                    fees.feeAmount
                );
                IERC20(fees.feeToken).approve(
-                    address(feeCollector),
+                    address(_feeCollector),
                    fees.feeAmount
                );
            } else {
                value = fees.feeAmount;
            }
-            feeCollector.collectFees{value: value}(fees, packedInfo, signature);
+            _feeCollector.collectFees{value: value}(fees, packedInfo, signature);

[G-14] Cache address(this) instead of re-calculate

Calculating address(this) takes some gas so it is better to cache it instead of re-calculating multiple times in same function. 2 instances

File : src/DecentBridgeExecutor.sol

30:        uint256 balanceBefore = weth.balanceOf(address(this));
31:        weth.approve(target, amount);
32:
33:        (bool success, ) = target.call(callPayload);
34:
35:        if (!success) {
36:            weth.transfer(from, amount);
37:            return;
38:        }
39:
40:        uint256 remainingAfterCall = amount -
41:            (balanceBefore - weth.balanceOf(address(this)));

30-41

File : src/DecentBridgeExecutor.sol

+       address _this = address(this);
-       uint256 balanceBefore = weth.balanceOf(address(this));
+       uint256 balanceBefore = weth.balanceOf(_this);
        weth.approve(target, amount);

        (bool success, ) = target.call(callPayload);

        if (!success) {
            weth.transfer(from, amount);
            return;
        }

        uint256 remainingAfterCall = amount -
-            (balanceBefore - weth.balanceOf(address(this)));
+            (balanceBefore - weth.balanceOf(_this));
File : src/UTBExecutor.sol

59:        uint initBalance = IERC20(token).balanceOf(address(this));
60:
61:        IERC20(token).transferFrom(msg.sender, address(this), amount);
62:        IERC20(token).approve(paymentOperator, amount);
63:
64:        if (extraNative > 0) {
65:            (success, ) = target.call{value: extraNative}(payload);
66:            if (!success) {
67:                (refund.call{value: extraNative}(""));
68:            }
69:        } else {
70:            (success, ) = target.call(payload);
71:        }
72:
73:        uint remainingBalance = IERC20(token).balanceOf(address(this)) -
74:            initBalance;

59-74

File : src/UTBExecutor.sol

+       address _this = address(this);
-       uint initBalance = IERC20(token).balanceOf(address(this));
+       uint initBalance = IERC20(token).balanceOf(_this);

-       IERC20(token).transferFrom(msg.sender, address(this), amount);
+       IERC20(token).transferFrom(msg.sender, _this, amount);
        IERC20(token).approve(paymentOperator, amount);

        if (extraNative > 0) {
            (success, ) = target.call{value: extraNative}(payload);
            if (!success) {
                (refund.call{value: extraNative}(""));
            }
        } else {
            (success, ) = target.call(payload);
        }

-        uint remainingBalance = IERC20(token).balanceOf(address(this)) -
+        uint remainingBalance = IERC20(token).balanceOf(_this) -
            initBalance;

[G-15] Refactor if-else statement to save gas

Since it is better to write less gas consuming condition before more gas consumer. In if statement due to operator short-circuiting. First is evaluated if 2nd is not needed then 2nd will not be evaluated in || OR && operations.

To save 2 not operations and 1 Sload if deliveryEth is false we can refactor this below code.

2 instances

File : src/DecentBridgeExecutor.sol

77:     if (!gasCurrencyIsEth || !deliverEth) {
78:           _executeWeth(from, target, amount, callPayload);
79:      } else {
80:           _executeEth(from, target, amount, callPayload);
81:      }

77-81

File : src/DecentBridgeExecutor.sol

-       if (!gasCurrencyIsEth || !deliverEth) {
-            _executeWeth(from, target, amount, callPayload);
-        } else {
-            _executeEth(from, target, amount, callPayload);
-        }
+       if (deliverEth && gasCurrencyIsEth) {
+            _executeEth(from, target, amount, callPayload);
+       } else {
+            _executeWeth(from, target, amount, callPayload);
+       }
File : src/DecentEthRouter.sol

272:        if (!gasCurrencyIsEth || !deliverEth) {
273:                weth.transfer(_to, _amount);
274:        } else {
275:                weth.withdraw(_amount);
276:                payable(_to).transfer(_amount);
277:        }

272-277

File : src/DecentEthRouter.sol

-        if (!gasCurrencyIsEth || !deliverEth) {
-             weth.transfer(_to, _amount);
-        } else {
-            weth.withdraw(_amount);
-            payable(_to).transfer(_amount);
-        }
+        if (deliverEth && gasCurrencyIsEth) {
+            weth.withdraw(_amount);
+            payable(_to).transfer(_amount); 
+        } else {
+             weth.transfer(_to, _amount);
+         }      

[G-16] Refactor function onOFTReceived to save gas

We can refactor this function to avoid 1 abi.decode. It will have same impact. 1 instance

File : src/DecentEthRouter.sol

245:    (uint8 msgType, address _from, address _to, bool deliverEth) = abi
246:            .decode(_payload, (uint8, address, address, bool));
247:
248:        bytes memory callPayload = "";
249:
250:        if (msgType == MT_ETH_TRANSFER_WITH_PAYLOAD) {
251:            (, , , , callPayload) = abi.decode(
252:                _payload,
253:                (uint8, address, address, bool, bytes)
254:            );

245-254

File : src/DecentEthRouter.sol

-        (uint8 msgType, address _from, address _to, bool deliverEth) = abi
-            .decode(_payload, (uint8, address, address, bool));

         bytes memory callPayload = "";

         if (msgType == MT_ETH_TRANSFER_WITH_PAYLOAD) {
-            (, , , , callPayload) = abi.decode(
-                _payload,
-                (uint8, address, address, bool, bytes)
-            );
+            (uint8 msgType, address _from, address _to, bool deliverEth, callPayload) = abi.decode(
+                _payload,
+                (uint8, address, address, bool, bytes)
+            );
+        }
+        else{
+             (uint8 msgType, address _from, address _to, bool deliverEth) = abi
+            .decode(_payload, (uint8, address, address, bool));
+        }

#0 - raymondfam

2024-01-26T15:04:13Z

All findings except G-16 are generically complementing the bot report.

#1 - c4-pre-sort

2024-01-26T15:04:18Z

raymondfam marked the issue as sufficient quality report

#2 - alex-ppg

2024-02-04T17:53:01Z

These findings were penalized:

  • G-08: Penalized as the functions do have a purpose; the difference in the arithmetic operations per adapter is a valid reason for multiple pure and external functions. To note, a staticcall is performed regardless and these functions can become view in the future, be view in a subset of adapters, etc.
  • G-10: Penalized as this is not gas-related. An event needs to be emitted, and the optimization would have to be applied at the dependency level which is OOS.

#3 - c4-judge

2024-02-04T17:53:06Z

alex-ppg marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter