1
2
3
4
5
6
7
8
9
10
11

function claimGovFees() public {
address[] memory assets = bondNFT.getAssets();

for (uint i=0; i < assets.length; i++) {
    uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
    IGovNFT(govNFT).claim(assets[i]);
    uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
    IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
    bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
    }
}

1

uint constant private DAY = 24 * 60 * 60;

1
2
3

(uint80 roundId, int256 assetChainlinkPriceInt, , uint256 updatedAt, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();
require(answeredInRound >= roundId, "price is stale");
require(updatedAt > 0, "round is incomplete");

• Use the safe variant and ERC721.mint
• Add an event for critical parameter changes
• Declare interfaces on separate files
• Constants should be upper case
• Replace constant private with private constant

When minting and burning tokens,the ERC1155Enumerable implementation does not correctly update the following states:

• uint256[] private _allTokens;
• mapping(uint256 => uint256) private _allTokensIndex;
• mapping(address => uint256) internal _currentIndex;

In particular:

• the _allTokens array length (and therefore the totalSupply()) always increases (never decreases)
• the _allTokensIndex[id] always increases
• the _curentIndex[from] always increases

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

contract HelperERC1155 is ERC1155Enumerable, ERC1155Holder {
    constructor() ERC1155("Test") {
    }
    function mint(uint256 id, uint256 amount) external {
        _mint(msg.sender, id, amount, bytes(""));
    }
    function burn(uint256 id, uint256 amount) external {
        _burn(msg.sender, id, amount);
    }
    function currentIndex(address owner) external view returns (uint256) {
        return _currentIndex[owner];
    }
    function allTokensIndex(uint256 id) external view returns (uint256) {
        return _allTokensIndex[id];
    }
    function allTokens(uint256 idx) external view returns (uint256) {
        return _allTokens[idx];
    }
    function idTotalSupply(uint256 id) external view returns (uint256) {
        return _idTotalSupply[id];
    }
}

1
2

-   pool.long0ProtocolFees = 0;
+   delete pool.long0ProtocolFees;

1
2
3
4
5
6
7
8
9
10

+    require(block.timestamp <= deadline, "KYCRegistry: signature expired");

    require(
    !kycState[kycRequirementGroup][user],
    "KYCRegistry: user already verified"
    );
-    require(block.timestamp <= deadline, "KYCRegistry: signature expired");
    bytes32 structHash = keccak256(
    abi.encode(_APPROVAL_TYPEHASH, kycRequirementGroup, user, deadline)
    );

1
2
3
4
5
6
7
8

-    for (uint256 i = 0; i < exCallData.length; ++i) {

+    for (uint256 i = 0; i < exCallData.length;) {
+      unchecked {
+        ++i;
+      }
    }
}

1
2
3
4
5

-    doTransferOut(admin, reduceAmount);
+    doTransferOut(payable(msg.sender), reduceAmount);
-    emit ReservesReduced(admin, reduceAmount, totalReservesNew);
+    emit ReservesReduced(msg.sender, reduceAmount, totalReservesNew);

1
2
3
4
5
6

-    if (fTokenToUnderlyingPrice[fToken] != 0) {
-      return fTokenToUnderlyingPrice[fToken];
+    uint256 fToken = fTokenToUnderlyingPrice[fToken];
+    if(fToken != 0) {
+      return fToken;   
    }

1
2

-  address public owner;
+  address public immutable owner;

1
2

-  mapping(address => bool) public allowedAsset;
+  mapping(address => uint256) public allowedAsset;

Recommendation:
import {contract1 , contract2} from "filename.sol";

1
2
3
4
5
6
7
8

function buy(uint256 _amount) external payable {
        ...
-       for (uint48 x = sale_.currentId + 1; x <= newId; x++) {
+       for (uint48 x = sale_.currentId; x < newId; ++x) {
            nft.mint(msg.sender, x);
        }
        ...
}

• Multiple access to mapping/array should use local variable cache
• Duplicated require should be modifier or function
• Use custom error rather than revert()/require()
• Use calldata instead of memory for read-only variable
• require()/revert() string longer than 32 bytes cost extra gas
• Using Openzeppelin Ownable2Step.sol is gas efficient
• Using UniswapV3 mulDiv function is gas-optimized
• Use nested if and, avoid multiple check combinations
• Avoid using state variable in emit (130 gas)
• Instead of cache a whole object ,try cache single Attributes
• Using int32 for time
• Don’t use _msgSender() if not supporting EIP-2771
• Using > 0 costs more gas than != 0 when used on a uint in a require() statement(version>0.8.13)
• Using bools for storage incurs overhead
• .length should not be looked up in every loop of a for-loop
• Using calldata instead of memory for read-only arguments in external functions saves gas
• Splitting require() statements that use && saves gas
• Use a more recent version of solidity
• require()/revert() strings longer than 32 bytes cost extra gas
• += costs more gas than = + for state variables
• Using storage instead of memory for structs/arrays saves gas

[H] Audit Daily Approved Tokens To A Pool Can Be Forced To Take

Code snippet

full code link

https://github.com

1
2
3
4
5
6
7
8
9
10

function take(
    address        borrowerAddress_,
    uint256        collateral_,
    address        callee_,
    bytes calldata data_
) external override nonReentrant {
    ...

    _transferQuoteTokenFrom(callee_, result.quoteTokenAmount);
}

Impact

Anyone who invokes the toke function and passes in someone else who approved this pool could take the tokens.

Proof of concept

I use foundry to make the whole test.

• 1.Firstly,we need to create our own ERC20 token and implement the quote contract.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

function setUp() public virtual {
    console.log("When transferring tokens...");
    alice = address(2);
    bob = address(3);

    myERC20 = new MyERC20();
    quote = new Quote(address(myERC20));

    //owner mint some token
    myERC20.mint(OWNERAMOUNT);

    //send some token to our dear testing user.
    myERC20.transfer(alice,SOMEAMOUNT);
    myERC20.transfer(bob,SOMEAMOUNT);
}

• 2.Use bob’s address approved the pool some token.Check the balance of bob after we make the call.

1
2
3
4
5

vm.prank(bob);
myERC20.approve(address(quote), SOMEAMOUNT);

//check bob balance step1.
assertEq(myERC20.balanceOf(bob), SOMEAMOUNT);

• 3.Use Alice’s address to call the take function which is public within the quote contract.

1
2
3

//alice transfer the approve tokens.
vm.prank(alice);
quote.take(address(bob), SOMEAMOUNT);

• 4.Finally, we check the balance of bob, if bob’s token has been token by anyone who makes the call.

1
2

//check bob balance step2.
assertEq(myERC20.balanceOf(bob), 0);

Recommended mitigation steps

Alternatively, consider checking that callee has approved spending quote tokens to msg.sender.

1
2
3
4
5
6
7
8
9
10

function take(
    address        borrowerAddress_,
    uint256        collateral_,
    address        callee_,
    bytes calldata data_
) external override nonReentrant {
    ...

    _transferQuoteTokenFrom(msg.sender, result.quoteTokenAmount);
}

[H] Audit Daily Selfdestruct May Cause The Funds To Be Lost

Code

https://github.com

1
2
3
4
5
6
7
8
9

function buy() external payable {
    _end();
}

function _end(Sale memory _sale) internal {
    emit End(_sale);
    ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
    selfdestruct(_sale.saleReceiver);
}

Impact

After the contract is destroyed,the subsequent execution of the contract’s function buy() is going on.That causes the msg.value token to be lost in the contract forever.

Proof of concept

Note:When there is no code at the address, the transaction will succeed, and the msg.value will be stored in the contract.

Let’s say Alice and bob are invoking the contact simultaneously. The transactions are sent to the mempool. Alice is finished executes her transaction when bob is still waiting for his result. And then the contract is destroyed.
Finally, bob finished his transaction and sent his token to this contract. This way bob’s token is lost and locked forever in this empty contract.

Recommended mitigation steps

Instead of using self-destruct, we could modify the state to represent the contract has completed the process.We could modify the code like this:

1
2
3
4
5
6

function _end(Sale memory _sale) internal {
        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
-       selfdestruct(_sale.saleReceiver);
+       sale.finalId = sale.currentId
+       sale.saleReceiver.transfer(address(this).balance);
}