Sushi Trident contest phase 1 - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

Bad use of pragma in the code

Note: this is a nitpick.

The contracts use a pragma solidity >=0.8.0. Example.

However, this is a bad practice.

This allows the code to be compiled across breaking versions of solidity. It may be true that 0.9.0 will have a change in semantics that will invalidate some contract functions. For example, compiling contracts written for 0.7.0 in 0.8.0 (assuming it compiles) will be problematic because of SafeMath.

Handle

hrkrshnn

Vulnerability details

Consider using custom errors instead of revert strings

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings. Use custom errors instead of revert strings for gas savings.

Note that this can be especially important for the factory contracts which deploys the underlying contracts. This is because custom errors take up less bytecode and therefore decreases the cost for the create2 deployment (each additional byte is 200 gas, as well as additional cost for memory expansion etc)

Handle

hrkrshnn

Vulnerability details

Caching the length in for loops

Consider a generic example of an array arr and the following loop:

for (uint i = 0; i < arr.length; i++) {
    // do something that doesn't change arr.length
}

In the above case, the solidity compiler will always read the length of the array during each iteration. That is,

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack):

uint length = arr.length;
for (uint i = 0; i < length; i++) {
    // do something that doesn't change arr.length
}

In the above example, the sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload, calldataload and dupN have the same gas cost, mload and calldataload needs an additional pushX value to put the offset in the stack, i.e., an extra 3 gas.

This optimization is especially important if it is a storage array or if it is a lengthy for loop. Note: this especially relevant for the IndexPool contract.

Note that the Yul based optimizer (not enabled by default; only relevant if you are using --experimental-via-ir or the equivalent in standard JSON) can sometimes do this caching automatically. However, this is likely not the case in your project. Reference.

Examples

./contracts/flat/BentoBoxV1Flat.sol:626:        for (uint256 i = 0; i < calls.length; i++) {
./contracts/pool/PoolDeployer.sol:34:            for (uint256 i; i < tokens.length - 1; i++) {
./contracts/pool/PoolDeployer.sol:36:                for (uint256 j = i + 1; j < tokens.length; j++) {
./contracts/pool/franchised/FranchisedIndexPool.sol:73:        for (uint256 i = 0; i < _tokens.length; i++) {
./contracts/pool/franchised/FranchisedIndexPool.sol:104:        for (uint256 i = 0; i < tokens.length; i++) {
./contracts/pool/franchised/FranchisedIndexPool.sol:132:        for (uint256 i = 0; i < tokens.length; i++) {
./contracts/pool/franchised/WhiteListManager.sol:105:        for (uint256 i = 0; i < merkleProof.length; i++) {
./contracts/pool/IndexPool.sol:71:        for (uint256 i = 0; i < _tokens.length; i++) {
./contracts/pool/IndexPool.sol:102:        for (uint256 i = 0; i < tokens.length; i++) {
./contracts/pool/IndexPool.sol:129:        for (uint256 i = 0; i < tokens.length; i++) {
./contracts/utils/TridentHelper.sol:27:        for (uint256 i = 0; i < data.length; i++) {
./contracts/TridentRouter.sol:65:        for (uint256 i; i < params.path.length; i++) {
./contracts/TridentRouter.sol:83:        for (uint256 i; i < path.length; i++) {
./contracts/TridentRouter.sol:123:        for (uint256 i; i < params.path.length; i++) {
./contracts/TridentRouter.sol:139:        for (uint256 i; i < params.initialPath.length; i++) {
./contracts/TridentRouter.sol:149:        for (uint256 i; i < params.percentagePath.length; i++) {
./contracts/TridentRouter.sol:157:        for (uint256 i; i < params.output.length; i++) {
./contracts/TridentRouter.sol:181:        for (uint256 i; i < tokenInput.length; i++) {
./contracts/TridentRouter.sol:223:        for (uint256 i; i < minWithdrawals.length; i++) {
./contracts/TridentRouter.sol:225:            for (; j < withdrawnLiquidity.length; j++) {
./contracts/TridentRouter.sol:274:        for (uint256 i; i < tokenInput.length; i++) {

Handle

hrkrshnn

Vulnerability details

Consider changing the _deployData architecture

Currently, the factory contracts implements deployment in the following fashion:

1. The argument to the external function has the type bytes memory.
2. This gets decoded using abi.decode(deploydata, args)

Example

Consider directly specifying the parameters.

modified   contracts/pool/ConstantProductPoolFactory.sol
@@ -10,8 +10,7 @@ import "./PoolDeployer.sol";
 contract ConstantProductPoolFactory is PoolDeployer {
     constructor(address _masterDeployer) PoolDeployer(_masterDeployer) {}

-    function deployPool(bytes memory _deployData) external returns (address pool) {
-        (address tokenA, address tokenB, uint256 swapFee, bool twapSupport) = abi.decode(_deployData, (address, address, uint256, bool));
+    function deployPool(address tokenA, address tokenB, uint256 swapFree, bool twapSupport) external returns (address pool) {
         if (tokenA > tokenB) {
             (tokenA, tokenB) = (tokenB, tokenA);
             _deployData = abi.encode(tokenA, tokenB, swapFee, twapSupport);

Advantages

1. Using _deployData instead of (address, address, uint256, uint256) needs larger calldata size. This is because the bytes also needs to store the length.
2. This avoids going via memory. For the byte array example, the value is first copied from calldata to memory (note: solidity currently does this in an inefficient for loop, instead of using calldatacopy; so if you really want to keep the architecture, writing it in assembly using calldatacopy would be even more efficient.) Effectively, this saves the cost to store in memory, as well as memory expansion costs. Since the function call does a contract deployment, the savings from memory expansion costs can be reasonably high, assuming that the quadratic pricing is reached.

This pattern seems to be used in several other places. All of them can be better optimized.

1. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/ConstantProductPool.sol#L124

Handle

hrkrshnn

Vulnerability details

Caching the storage read to tokens.length

Ignoring the caching for the for-loop condition (see my other issue "Caching the length in for loops"), this would save an additional sload, around 100 gas.

Context.

modified   contracts/pool/IndexPool.sol
@@ -121,12 +121,12 @@ contract IndexPool is IPool, TridentERC20 {
         (address recipient, bool unwrapBento, uint256 toBurn) = abi.decode(data, (address, bool, uint256));

         uint256 ratio = _div(toBurn, totalSupply);
-        withdrawnAmounts = new TokenAmount[](tokens.length);
+        uint length = tokens.length;
+        withdrawnAmounts = new TokenAmount[](length);

         _burn(address(this), toBurn);

-        for (uint256 i = 0; i < tokens.length; i++) {
+        for (uint256 i = 0; i < length; i++) {
             address tokenOut = tokens[i];
             uint256 balance = records[tokenOut].reserve;
             uint120 amountOut = uint120(_mul(ratio, balance));

Handle

hrkrshnn

Vulnerability details

Caching a storage load in TridentERC20

Context

This avoids an unnecessary sload.

modified   contracts/pool/TridentERC20.sol
@@ -73,8 +73,9 @@ abstract contract TridentERC20 {
         address recipient,
         uint256 amount
     ) external returns (bool) {
-        if (allowance[sender][msg.sender] != type(uint256).max) {
-            allowance[sender][msg.sender] -= amount;
+        uint _allowance = allowance[sender][msg.sender];
+        if (_allowance != type(uint256).max) {
+            allowance[sender][msg.sender] = _allowance - amount;
         }
         balanceOf[sender] -= amount;
         // @dev This is safe from overflow - the sum of all user

Handle

hrkrshnn

Vulnerability details

Unused state variable barFee and _barFeeTo in IndexPool

The public state variable https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/IndexPool.sol#L43 and https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/IndexPool.sol#L20 seems to be never used anywhere in the contract. Removing them will save gas for (almost) all external calls because of their presence in function dispatch.

Handle

hrkrshnn

Vulnerability details

Use calldata instead of memory for function parameters

In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

contract C {
    function add(uint[] memory arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
    function add(uint[] calldata arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

Examples

Note: The following pattern is prevalent in the codebase:

function f(bytes memory data) external {
    (...) = abi.decode(data, (..., types, ...));
}

Here, changing to bytes calldata will decrease the gas. The total savings for this change across all such uses would be quite significant.

Examples:

./contracts/examples/PoolFactory.sol:15:    function deployPool(bytes memory _deployData) external override returns (address) {
./contracts/examples/PoolTemplate.sol:12:    constructor(bytes memory _data) {
./contracts/flat/BentoBoxV1Flat.sol:603:    function _getRevertMsg(bytes memory _returnData) internal pure returns (string memory) {
./contracts/pool/HybridPoolFactory.sol:13:    function deployPool(bytes memory _deployData) external returns (address pool) {
./contracts/pool/IndexPoolFactory.sol:13:    function deployPool(bytes memory _deployData) external returns (address pool) {
./contracts/pool/franchised/FranchisedConstantProductPool.sol:54:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/pool/franchised/FranchisedHybridPool.sol:60:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/pool/franchised/FranchisedIndexPool.sol:61:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/pool/HybridPool.sol:60:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/pool/ConstantProductPoolFactory.sol:13:    function deployPool(bytes memory _deployData) external returns (address pool) {
./contracts/pool/ConstantProductPool.sol:54:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/pool/IndexPool.sol:61:    constructor(bytes memory _deployData, address _masterDeployer) {
./contracts/utils/TridentHelper.sol:143:    function getSelector(bytes memory _data) internal pure returns (bytes4 sig) {

Other examples:

1. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L174
2. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L218

Handle

hrkrshnn

Vulnerability details

Consider putting some parts of _div in unchecked

Context

modified   contracts/pool/IndexPool.sol
@@ -332,7 +332,8 @@ contract IndexPool is IPool, TridentERC20 {

     function _div(uint256 a, uint256 b) internal pure returns (uint256 c2) {
         uint256 c0 = a * BASE;
-        uint256 c1 = c0 + (b / 2);
+        unchecked { uint256 tmp =  b / 2; }
+        uint256 c1 = c0 + tmp;
         c2 = c1 / b;
     }

Looking at the optimized assembly generated, the unchecked version doesn't seem to have the additional check for division by zero. I'm not entirely sure why, but my guess is because of inlining. Also consider replacing the division by inline assembly.

uint tmp;
assembly {
    tmp := div(b, 2)
}
uint256 c1 = c0 + tmp;

The change avoids an if condition which checks if the divisor is zero, which the optimizer is currently unable to optimize out. The gas savings would be around 16 (reduces a jumpi, push 0 and dupN). Since the _div function is called from throughout the code (also present in other contracts), this may be worth considering, although I admit this might be too much of a micro-optimization.

Handle

hrkrshnn

Vulnerability details

The functions refundETH and unwrapWETH is generalized-front-runnable

Context:

1. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L309
2. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L304

In particular, refundETH is very easily front-runnable, simply copy the calldata from mempool and get your transaction first. For unwrapWETH, the front-runner simply has to change the recipient address to their own address.

Although, this doesn't affect any part of the main contract logic, this possibility effectively makes the refund process for an average user almost impossible. The only certain way to get a refund is to use a service flashbots or taichi.

Recommended Mitigation Steps

There are two ways to resolve this.

1. Make it clear in the documentation that one should not to call these functions directly and instead use a private mempool service.
2. Make the withdrawal a two-step process. That is, a user can first claim, followed by the actual call.