Blur Exchange contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

Vulnerability details

Impact

The StandardPolicyERC1155 contract always returns 1 for the amount return value. Thus, when fulfilling an order, the exchange only transfers one token from the seller to the buyer, not the value specified in the order.

The user still pays the full price but receives fewer tokens than expected. This is a direct loss of funds and will occur for every ERC1155 order where amount > 1.

Proof of Concept

The user can specify the number of tokens in the order here: https://github.com/code-423n4/2022-10-blur/blob/main/contracts/lib/OrderStructs.sol#L19

The actual number of tokens that is sent when the order is fulfilled is determined using _canMatchOrder(): https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L145

The function calls the respective policy, in our case StandardPolicyERC1155: https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L422-L430

The policy's functions always return 1 for the amount return value instead of order.amount: https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

Thus, in the end, only one token is sent to the buyer no matter what the value of order.amount was. The value is ignored throughout the whole exchange contract.

Tools Used

none

Recommended Mitigation Steps

Check that the amount value of both orders matches and then return that value instead of 1.

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/ExecutionDelegate.sol#L16 https://github.com/code-423n4/2022-10-blur/blob/main/scripts/deploy.ts#L18 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L29 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/ExecutionDelegate.sol#L36 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/ExecutionDelegate.sol#L120

Vulnerability details

Impact

The ExecutionDelegate contract's owner is set to the deployer's address by default. The owner being a hot wallet brings a big security risk. It's likely that the private key will sit on the machine of one of the devs. That is not as secure as having the owner be a multisig or even a timelock contract.

Here are two recent examples of admin keys being compromised that I remember off the top of my head:

https://roninblockchain.substack.com/p/back-to-building-ronin-security-breach https://www.certik.com/resources/blog/2QRuMEEZAWHx0f16kz43uC-harmony-incident-analysis

The risk of compromised keys is very real. It has nothing to do with smart contracts where the main focus of audits lies so it's ignored most of the time. But, if the ExecutionDelegate owner's keys are compromised, the attacker can approve a new contract that is allowed to access the transfer functions. The exchange's users are supposed to approve their WETH tokens to the contract which the attacker can then all steal.

Proof of Concept

ExecutionDelegate imports OZ's Ownable contract:

contract ExecutionDelegate is IExecutionDelegate, Ownable {}

The Ownable contract will set the msg.sender as the owner in its constructor:

    constructor() {
        _transferOwnership(_msgSender());
    }

If the key is compromised, the attacker can approve a malicious contract using approveContract():

    function approveContract(address _contract) onlyOwner external {
        contracts[_contract] = true;
        emit ApproveContract(_contract);
    }

They can then use the contract to call transferERC20() for each user that has approved any tokens to the contract to steal them:

    function transferERC20(address token, address from, address to, uint256 amount)
        approvedContract
        external
        returns (bool)
    {
        require(revokedApproval[from] == false, "User has revoked approval");
        return IERC20(token).transferFrom(from, to, amount);
    }

Tools Used

none

Recommended Mitigation Steps

There's a hardhat task to transfer ownership to the DAO's address. That should be done for the ExecutionDelegate contract within the deployment task. https://github.com/code-423n4/2022-10-blur/blob/main/scripts/deploy.ts#L183

Gas Report

• G-01: use uint256 for reentrancy guard
• G-02: ExecutionDelegate's own approval system is a waste of gas
• G-03: use unchecked when incrementing a loop's iterator
• G-04: use ++i instead of i++
• G-05: incrementNonce() can be rewritten to save a SLOAD

G-01: use uint256 for reentrancy guard

Storing a bool to storage is more expensive than an uint256 because the bool has to be packed to 32 bytes.

For a gas optimized ReentrancyGuard contract, check out the solmate: https://github.com/transmissions11/solmate/blob/main/src/utils/ReentrancyGuard.sol

G-02: ExecutionDelegate's own approval system is a waste of gas

To approve the contract to access your funds you have to use the ERC20 token's approval system & the ExecutionDelegate's own one. Having that additional approval layer is a waste of gas and doesn't really provide any more protection.

You'd save a SLOAD with every transfer.

G-03: use unchecked when incrementing a loop's iterator

The value can't realistaclly overflow because of the loop's condition. Thus you can use unchecked to save the opcodes used for the overflow check.

./contracts/PolicyManager.sol:77:        for (uint256 i = 0; i < length; i++) {
./contracts/lib/MerkleVerifier.sol:38:        for (uint256 i = 0; i < proof.length; i++) {
./contracts/lib/EIP712.sol:77:        for (uint256 i = 0; i < fees.length; i++) {
./contracts/BlurExchange.sol:199:        for (uint8 i = 0; i < orders.length; i++) {
./contracts/BlurExchange.sol:476:        for (uint8 i = 0; i < fees.length; i++) {

G-04: use ++i instead of i++

i++ saves the original value in memory and returns that while ++i just returns the new value. By not saving the original value to memory you save a little gas.

./contracts/PolicyManager.sol:77:        for (uint256 i = 0; i < length; i++) {
./contracts/lib/MerkleVerifier.sol:38:        for (uint256 i = 0; i < proof.length; i++) {
./contracts/lib/EIP712.sol:77:        for (uint256 i = 0; i < fees.length; i++) {
./contracts/BlurExchange.sol:199:        for (uint8 i = 0; i < orders.length; i++) {
./contracts/BlurExchange.sol:476:        for (uint8 i = 0; i < fees.length; i++) {

G-05: incrementNonce() can be rewritten to save a SLOAD

Instead of

function incrementNonce() external {
    nonces[msg.sender] += 1;
    emit NonceIncremented(msg.sender, nonces[msg.sender]);
}

you can do:

function incrementNonce() external {
    emit NonceIncremented(msg.sender, ++nonces[msg.sender]);
}

Same principle as explained in the above issue.