Sushi Trident contest phase 1 - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function _update() of ConstantProductPool.sol verifies if blockTimestampLast == 0. The value of blockTimestampLast is also passed in the parameter _blockTimestampLast (note: with extra _ ) So _blockTimestampLast could also be used, which saves a bit of gas.

Proof of Concept

https://github.com/sushiswap/trident/blob/master/contracts/pool/ConstantProductPool.sol#L264 function _update(... uint32 _blockTimestampLast) internal { ... if (blockTimestampLast == 0) { // could also use _blockTimestampLast

Tools Used

Recommended Mitigation Steps

replace if (blockTimestampLast == 0) {
with if (_blockTimestampLast == 0) {

Handle

gpersoon

Vulnerability details

Impact

In the function _powApprox of IndexPool.sol a few gas savings are possible, see suggestion below.

Proof of Concept

https://github.com/sushiswap/trident/blob/master/contracts/pool/IndexPool.sol#L286

Tools Used

Recommended Mitigation Steps

Suggestion for optimization: function _powApprox(uint256 base, uint256 exp, uint256 precision) internal pure returns (uint256 sum) { uint256 a = exp; (uint256 x, bool xneg) = _subFlag(base, BASE); uint256 term = BASE; sum = term; bool negative; uint256 bigK=0; // initially 0 for (uint256 i = 1; term >= precision; i++) { (uint256 c, bool cneg) = _subFlag(a, bigK)); // now bigK = (i-1) * BASE; term = _mul(term, _mul(c, x)); bigK += BASE; // now bigK = i * BASE; term = _div(term, bigK); if (term == 0) break; negative = negative != xneg; // more predictable gas usage & slightly lower gas negative = negative != cneg; // more predictable gas usage & slightly lower gas sum = (negative ? sum - term : sum + term); // slightly less instructions } }

Handle

gpersoon

Vulnerability details

Impact

The callback functions in TridentRouter.sol check "msg.sender == cachedPool" on the function entry. At the end of the function cachedMsgSender is reset to 1. However cachedPool is not reset to 1. This means tridentSwapCallback / tridentMintCallback could be called again. I couldn't find a scenario to misuse this, but maybe setting cachedPool to address(1) is worth the trouble.

Proof of Concept

https://github.com/sushiswap/trident/blob/master/contracts/TridentRouter.sol#L19 /// @dev Used to ensure that tridentSwapCallback is called only by the authorized address. /// These are set when someone calls a flash swap and reset afterwards. address internal cachedMsgSender; address internal cachedPool;

function tridentSwapCallback(bytes calldata data) external { require(msg.sender == cachedPool, "UNAUTHORIZED_CALLBACK"); ... cachedMsgSender = address(1); }

function tridentMintCallback(bytes calldata data) external {
   require(msg.sender == cachedPool, "UNAUTHORIZED_CALLBACK");
     ...
    cachedMsgSender = address(1);
}

Tools Used

Recommended Mitigation Steps

Consider also setting cachedPool to address(1) at the end of the functions tridentSwapCallback and tridentMintCallback