Sublime contest - 0xngndev's results

Handle

0xngndev

Vulnerability details

Impact

Reduce code size and gas expenditure by upgrading the compiler version to version 0.8.0 or higher in order to remove SafeMath from your contracts.

I was curious to test how much more expensive both in gas and code size was to use SafeMath contrary to simply using a compiler version higher than 0.8.0. So I wrote a small contract with a function that performed:

• Addition
• Substraction
• Multiplication
• Division

And using Dapptools I tested gas usage and code size first using 0.8.10 compiler version without SafeMath, and then using SafeMath.

Results: Without SafeMath:

• Gas spent: 1265
• Code size: 514 bytes

Using SafeMath:

• Gas spent: 1691
• Code size: 926 bytes

With these results in mind, I would suggest upgrading your compiler version if possible and stop using SafeMath as it's not necessary with compiler version higher than 0.8.0. You can also make use of unchecked in higher versions to save even more gas if you are certain the arithmetic operation you

Proof of Concept

Here is the example contract I ran to test this:

• Without SafeMath

pragma solidity 0.8.10;

contract SafeMath {
  function add(uint256 x, uint256 y)
    public
    pure
    returns (
      uint256,
      uint256,
      uint256,
      uint256
    )
  {
    uint256 addResult = x + y;
    uint256 mulResult = x * y;
    uint256 divResult = x / y;
    uint256 substractResult = x - y;
    return (addResult, mulResult, divResult, substractResult);
  }
}

• With SafeMath

pragma solidity ^0.7.6;

import "@openzeppelin/contracts/math/SafeMath.sol";

//inlined saves gas in the calling function

contract SafeMath {
  using SafeMath for uint256;

  function add(uint256 x, uint256 y)
    public
    pure
    returns (
      uint256,
      uint256,
      uint256,
      uint256
    )
  {
    uint256 addResult = x.add(y);
    uint256 mulResult = x.mul(y);
    uint256 divResult = x.div(y);
    uint256 substractResult = x.sub(y);
    return (addResult, mulResult, divResult, substractResult);
  }
}

And here is the test file

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../SafeMath.sol";

contract SafeMathTest is DSTest {
  SafeMath safeMathContract;

  function setUp() public {
    safeMathContract = new SafeMath();
  }

  //concrete test
  function testAdd() public logs_gas {
    safeMathContract.add(10, 5);
  }
}

Tools

DappTools

Handle

0xngndev

Vulnerability details

Impact

Compiler detecs unused variables, function parameters and a function that can be restricted to pure. I'll enumerate them here:

• SavingsAccount.sol L209 —> _receivedToken is unused
• SavingsAccount.sol L246 —> _receivedToken is unused
• NoYield.sol L152 —> function parameter address _asset is unused
• NoYield.sol L162 —> function parameter address _asset is unused
• NoYield.sol L53 —> liquidityToken function visibility can be restricted to pure

Mitigation Steps

Simply remove the unused variables and functions parameters, and change the function's visibility.

Handle

0xngndev

Vulnerability details

Impact

Some of your contracts are quite large byte-wise and require the optimizer with low runs to not reach the code size limit of 24576 bytes. The code size of these contracts can be drastically reduced by shortening the length of your error messages, reducing their deployment cost.

The best example of contracts having unnecessary long erorr messages are CreditLine.sol and PoolFactory.sol. When it comes to factories, whose primary goal is to deploy contracts, reducing the cost of doing so is something to keep in mind.

Mitigation Steps

Reduce the length of your error strings. Error messages like:

'PoolFactory::createPool - Repayment interval not within limits'

Could be reduced to:

Interval out of limits

You could save even more by doing something similar to what Uniswap does. They have very short error messages like: ST, and they expand on what they mean in their documentation.

Another approach is to use revert with CustomErrors, something like this:

if (....) revert CustomError()

Following the example I used above, you could have a custom error message that says:

OutOfBounds() and expand what it means in the natspec.

Custom error messages are cheaper than strings error messages. Here's a snippet of Solidity's documentation about this:

Using a custom error instance will usually be much cheaper than a string description, because you can use the name of the error to describe it, which is encoded in only four bytes. A longer description can be supplied via NatSpec which does not incur any costs.

Handle

0xngndev

Vulnerability details

Impact

Reduce code size and gas expenditure by inlining internal functions that are only used once throughout your contract.

In some of your contracts, like AaveYield.sol, you have an initialize function that calls the internal functions updateSavingsAccount and updateAaveAddresses. These two functions are only called in the initialize function, so you can save some gas and code size by simply inlining their logic inside initialize. The tradeoff, of course, is that you lose readability, and because initialize will only be called once, perhaps it's not worth the tradeoff.

To test the difference in code size and gas expenditure I wrote a example contract replicating the behaviour to see how much cheaper inlining the logic was.

The results:

Inlining the logic:

• Gas spent: 45155
• Code size: 453 bytes

Separating into internal functions:

• Gas spent: 45189
• Code size: 467 bytes

Proof of Concept

Contracts I used to test the gas expenditure and code size differences:

• InliningLogic

//SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract InliningLogic {
  address public someRandomAddress;
  address public anotherRandomAddress;

  function assignAddresses(
    address _someRandomAddress,
    address _anotherRandomAddress
  ) public {
    someRandomAddress = _someRandomAddress;
    anotherRandomAddress = _anotherRandomAddress;
  }
}

• InliningLogic.t —> DappTools test file

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../InliningLogic.sol";

contract InliningLogicTest is DSTest {
  InliningLogic inliningLogicContract;

  function setUp() public {
    inliningLogicContract = new InliningLogic();
  }

  function testInliningLogic() public logs_gas {
    inliningLogicContract.assignAddresses(address(0x1), address(0x1));
  }
}

• NotInliningLogic

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

contract NotInliningLogic {
  address public someRandomAddress;
  address public anotherRandomAddress;

  function assignAddresses(
    address _someRandomAddress,
    address _anotherRandomAddress
  ) public {
    _assignAddresses(_someRandomAddress, _anotherRandomAddress);
  }

  function _assignAddresses(
    address _someRandomAddress,
    address _anotherRandomAddress
  ) internal {
    someRandomAddress = _someRandomAddress;
    anotherRandomAddress = _anotherRandomAddress;
  }
}

• NotInliningLogic.t —> test file

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../NotInliningLogic.sol";

contract NotInliningLogicTest is DSTest {
  NotInliningLogic notInliningLogicContract;

  function setUp() public {
    notInliningLogicContract = new NotInliningLogic();
  }

  function testNotInliningLogic() public logs_gas {
    notInliningLogicContract.assignAddresses(address(0x1), address(0x1));
  }
}

Tools

DappTools

Handle

0xngndev

Vulnerability details

Impact

In NoYield.sol the natspec of function liquidityToken says it's used to query the liquidity token for a given asset. However, the function simply returns the address of the passed in asset, not its liquidity token address. This could be because NoYield.sol does not have liquidity tokens and so it simply returns the same asset address the function is called with. Either way, I found it slightly confusing, and I'm not fully sure if I understand why this function is there other than to fulfill the interface.

There's also a small typo in the @return of the natspec:

@return _tokenAddress address of the lqiudity token for the asset

should be:

@return _tokenAddress address of the liquidity token for the asset