Detecting MISO and Opyn’s msg.value reuse vulnerability with Slither

By Simone Monica

On August 18, 2021, samczsun reported a critical vulnerability in SushiSwap’s MISO smart contracts, which put ~350 million USD (109 thousand ETH) at risk. This issue is similar to an attack that was conducted on the Opyn codebase in August of 2020.

At the time of the report, I was finishing my blockchain security apprenticeship at Trail of Bits, where I learned more about Slither’s capabilities. I immediately wondered whether it was possible to create Slither detectors for these vulnerabilities. The answer is yes! Today, we are releasing two new open-source detectors that can detect the Opyn and MISO vulnerabilities, respectively: msg-value-loop and delegatecall-loop.

msg.value inside a loop? PLS NO

The underlying result of both the MISO and Opyn vulnerabilities is the same: the reuse of the same msg.value amount multiple times. The difference is that the msg.value is used explicitly (msg.value) in Opyn and implicitly (delegatecall) in MISO.

Let’s look at a simple example to demonstrate the vulnerability.

In Opyn’s case, msg.value is used inside a loop in a payable function. If addBalances() is called with multiple receivers, the same msg.value will be reused for each recipient even though the corresponding ETH for only one recipient is sent.

contract C {
  mapping (address => uint256) balances;                
  function addBalances(address[] memory receivers) public payable {
            for (uint256 i = 0; i < receivers.length; i++) {
                                balances[receivers[i]] += msg.value;
}
  }
}

In MISO’s case, the source of the vulnerability is a delegatecall inside a loop within a payable function that also calls a payable function. Delegatecall makes a call to a function maintaining the current contract’s context, sender, and value. This is a simplified explanation of the MISO vulnerability; for more detail, I suggest that you read samczsun's blog post.


contract C {
  mapping (address => uint256) balances;
 
  function addBalance(address a) public payable {
                balances[a] += msg.value;
  }         
 
  function addBalances(address[] memory receivers) public payable {
                for (uint256 i = 0; i < receivers.length; i++) {
                                address(this).delegatecall(abi.encodewithsignature(“addBalance(address)”, receivers [i]));
}
  }
}

Slither's new detectors

By running Slither to detect calls to delegatecall and msg.value in a loop, we get the following results:

$ slither --detect delegatecall-loop Delegatecall.sol
C.addBalances(address[]) (delegatecall.sol#10-15) has delegatecall inside a loop in a payable function: address(this).delegatecall(abi.encodeWithSignature(addBalance(address),receivers[i])) (delegatecall.sol#12)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#payable-functions-using-delegatecall-inside-a-loop
Delegatecall.sol analyzed (1 contracts with 1 detectors), 1 result(s) found
 
$ slither --detect msg-value-loop Msgvalue.sol
C.addBalances(address[]) (msgvalue.sol#7-12) use msg.value in a loop:
balances[receivers[i]] += msg.value (msgvalue.sol#9)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#msgvalue-inside-a-loop
Msgvalue.sol analyzed (1 contracts with 1 detectors), 1 result(s) found

These two detectors are implemented with the same logic using the CFG representation and Slither’s intermediate language representation, SlithIR. The detectors iterate through the contracts’ payable function nodes and check whether the current node is entering or exiting a loop. Now, SlithIR comes to our aid. The two detectors’ implementations diverge, and they iterate through the node’s SlithIR operations; msg-value-loop checks whether the current operation reads msg.value (see the detector code here), and delegatecall-loop checks whether the current operation is a delegatecall (see the detector code here).

Let’s try the detectors on the smart contracts that were found to be vulnerable.

Opyn

$ slither 0x951D51bAeFb72319d9FBE941E1615938d89ABfe2 --detect msg-value-loop
OptionsContract._exercise(uint256,address) (crytic-export/etherscan-contracts/0x951D51bAeFb72319d9FBE941E1615938d89ABfe2-oToken.sol#1816-1899) use msg.value in a loop: require(bool,string)(msg.value == amtUnderlyingToPay,Incorrect msg.value) (crytic-export/etherscan-contracts/0x951D51bAeFb72319d9FBE941E1615938d89ABfe2-oToken.sol#1875)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#msgvalue-inside-a-loop
0x951D51bAeFb72319d9FBE941E1615938d89ABfe2 analyzed (13 contracts with 1 detectors), 1 result(s) found

SushiSwap’s MISO

$ slither 0x4c4564a1FE775D97297F9e3Dc2e762e0Ed5Dda0e --detect delegatecall-loop
BaseBoringBatchable.batch(bytes[],bool) (contracts/Utils/BoringBatchable.sol#35-44) has delegatecall inside a loop in a payable function: (success,result) = address(this).delegatecall(calls[i]) (contracts/Utils/BoringBatchable.sol#39)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#payable-functions-using-delegatecall-inside-a-loop
0x4c4564a1FE775D97297F9e3Dc2e762e0Ed5Dda0e analyzed (21 contracts with 1 detectors), 1 result(s) found

Conclusion

In summary, Slither is a powerful tool for preventing security vulnerabilities in smart contracts, and it can be expanded by creating new detectors. If you want to learn more about this tool, check out our "Building Secure Smart Contracts" guide.

My apprenticeship at Trail of Bits has been fun and has helped me improve my security skills and learn how to better approach audits. If you are interested in having a similar experience, you can apply to join us as a blockchain security apprentice.